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

Visitor: fix description of "ancestors" arg + test #1250

Merged
merged 2 commits into from
May 16, 2018

Conversation

IvanGoncharov
Copy link
Member

This description is incorrect:

// All nodes and Arrays visited before reaching this node.
// These correspond to array indices in `path`.
// Note: ancestors includes arrays which contain the visited node.
ancestors: $ReadOnlyArray<TAnyNode | $ReadOnlyArray<TAnyNode>>,

Since ancestors never includes parent, and it's already tested here:

expect(ancestors.length).to.equal(path.length - 1);

But I added a separate test to make it more obvious.

@IvanGoncharov
Copy link
Member Author

@leebyron Can you please review this PR?

@IvanGoncharov
Copy link
Member Author

@leebyron Can you please take a look?

enter(node, key, parent, path, ancestors) {
const inArray = typeof key === 'number';
const expectedAncestors = nodesInPath.slice(0, path.length - 1);
expect(ancestors).to.deep.equal(expectedAncestors);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not expect(ancestors).to.deep.equal(nodesInPath);? i.e. why is the slicing needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone Because we need nodesInPath without last element and slice(0, path.length - 1) does exactly that it returns an array without last element.

Basically, it tests this statement:

All nodes and Arrays visited before reaching parent of this node

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjmahone I rewrote this test to make it more explicit.

@IvanGoncharov IvanGoncharov force-pushed the fixAncestorsDescription branch from 7fae89b to b446116 Compare May 14, 2018 21:40
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

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

Thanks for figuring out this specification, and improving the docs for it!

@mjmahone mjmahone merged commit 4aca994 into graphql:master May 16, 2018
@IvanGoncharov IvanGoncharov deleted the fixAncestorsDescription branch May 16, 2018 20:39
@IvanGoncharov
Copy link
Member Author

@mjmahone It was my pleasure.

I think now visit coverage is near 100% plus its interface is fully documented so next step will be to work on simplifying this function as suggested in #1145 (review)

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

Successfully merging this pull request may close these issues.

3 participants