Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add depth check to oneNoteItemUtils #118

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JaneHazen
Copy link
Contributor

Add two methods to check depth of notebooks and sections in oneNoteItemUtils

Add two methods to check depth of notebooks and sectinos in oneNoteItemUtils
@JaneHazen JaneHazen requested review from mttwc and jogonzal April 27, 2018 22:57
@@ -6,6 +6,8 @@ import { Polyfills } from '../polyfills';

Polyfills.find();

export type noteBookOrSectionGroup = Notebook | SectionGroup;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit- notebook isn't two words, therefore noteBook -> notebook

* Finds the maximum depth of SectionGroup or Notebook,
* includes the number of sections below it
*/
getDepthOfParent(parent: noteBookOrSectionGroup ): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be private?

/**
* Finds the maximum depth of notebooks list, including sections
*/
getDepthOfNotebooks(notebooks: Notebook[]): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this being used?

let maxDepth = containsAtLeastOneSection ? 1 : 0;

if (parent.sectionGroups) {
for (let i = 0; i < parent.sections.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant parent.sectiongroups here. Also, I suggest using
for(const sectionGroup of parent.sectionGroups){
...
instead of a for with indexes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sectionGroups thing is actually to measure the sections here- it looks weird but changing these around breaks it. I'm mostly copying this code over from the utils in the OneNoteAPI repo we were using before

Copy link
Contributor

@jogonzal jogonzal Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code says:


if (parent.sectionGroups) {
for (let i = 0; i < parent.sections.length; i++) {
maxDepth = Math.max(this.getDepthOfParent(parent.sectionGroups[i]), maxDepth);
}
}

Let's suppose this parent has 1 section groups and 10 sections. It goes inside the if, loops from 0 to 9, and tries to access parent.sectionGroups[i]; It will throw when i=1 because the parent only has 1 section group.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean there's a bug in the OneNoteApi repo we should fix?

/**
* Finds the maximum depth of notebooks list, including sections
*/
getDepthOfNotebooks(notebooks: Notebook[]): number {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to unit test this method

JaneHazen and others added 4 commits April 28, 2018 12:18
Add tests, fix variable name
Add getPathFromNotebooksToSection and tests
Use GetAncestry instead of NotebookParentPath, fix tests
import { Section } from '../../src/oneNoteDataStructures/section';

describe('OneNoteItemUtils', () => {
const notebook1: Notebook = {
Copy link
Contributor

@jogonzal jogonzal Apr 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a number of entities here that your test use throughout several tests here. I'll simplify a bit:

const notebook1: Notebook = {
... sections: [] ...
}
it('test1', () => {
        notebook1.push(section1);
		const depthOfNotebooks = OneNoteItemUtils.getDepthOfNotebooks([notebook1]);
		expect(depthOfNotebooks).toEqual(2);
	});
it('test2', () => {
        const depthOfNotebooks = OneNoteItemUtils.getDepthOfNotebooks([notebook1]);
		expect(depthOfNotebooks).toEqual(1);
	});

If both tests run in the same persisted browser instance, then the order in which the tests run affects whether they pass or not, because they both use notebook1. A better way to build this could be:

function createNewNotebook(){
  return new {
    ... sections: [] ...
  }
}
it('test1', () => {
        var notebookMock = createNotebookMock();
        notebook1.push(section1);
		const depthOfNotebooks = OneNoteItemUtils.getDepthOfNotebooks([notebookMock]);
		expect(depthOfNotebooks).toEqual(2);
	});
it('test2', () => {
        const depthOfNotebooks = OneNoteItemUtils.getDepthOfNotebooks([createNotebookMock()]);
		expect(depthOfNotebooks).toEqual(1);
	});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also have a parameter to specify the notebook name (if you wish to) in createNewNotebook().

Copy link
Contributor

@jogonzal jogonzal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with a suggestion on how to structure tests

Edit oneNoteItemUtils
@mttwc
Copy link
Contributor

mttwc commented May 3, 2018

This PR doesn't have any changes. Was it already merged into master via another branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants