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

- fix Comment.associate for postfix conditions/loops #688

Merged
merged 2 commits into from May 22, 2020

Conversation

@marcandre
Copy link
Contributor

marcandre commented May 15, 2020

and also any node with children in inverted order.

See test file for example.

An faster alternative solution would be to instead figure out which nodes might have such a situation and visit them accordingly (are there any other than postfix if/unless/while?), but this slower solution seemed the most robust.

@marcandre
Copy link
Contributor Author

marcandre commented May 15, 2020

For reference: the comment in the new test used to be associated to the (send nil :some_condition) node.

@marcandre marcandre marked this pull request as draft May 17, 2020
@marcandre
Copy link
Contributor Author

marcandre commented May 17, 2020

Now that I think of it, the tests suite of parser itself can be used to find those cases and make for a more efficient implementation. Stay tuned for an improved PR :-)

@marcandre marcandre force-pushed the marcandre:fix_associator branch from 94adecc to c72e614 May 18, 2020
@marcandre
Copy link
Contributor Author

marcandre commented May 18, 2020

I've amended the PR to be more efficient. It uses the full test suite to double check that the efficiency isn't at the expense of correctness. I added a method Node#children_in_source_order; let me know if you'd prefer it to be hidden somewhere in the CommentAssociator.

@marcandre marcandre marked this pull request as ready for review May 18, 2020
lib/parser/ast/node.rb Outdated Show resolved Hide resolved
@marcandre marcandre force-pushed the marcandre:fix_associator branch from c72e614 to 27c772d May 20, 2020
Add `Node#children_in_source_order`
@marcandre marcandre force-pushed the marcandre:fix_associator branch from 27c772d to 0c83e21 May 20, 2020
@marcandre
Copy link
Contributor Author

marcandre commented May 20, 2020

Hopefully third time's a charm...
I refactored the test helper to make it cleaner to check something against all constructed nodes.
That made it easy to put the children_in_source_order as a private method of the associator and makes the tests nicer and isolated.

@iliabylich iliabylich changed the title Fix Comment.associate for postfix if - fix Comment.associate for postfix conditions/loops May 22, 2020
@iliabylich iliabylich merged commit de200f1 into whitequark:master May 22, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iliabylich
Copy link
Collaborator

iliabylich commented May 22, 2020

@marcandre Thanks a lot!

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.