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

+ Source::TreeRewriter: Add #merge, #merge! and #empty? #674

Merged
merged 1 commit into from Apr 14, 2020

Conversation

marcandre
Copy link
Contributor

This PR adds methods for merging Source::TreeRewriters.

There is also a simple method to check if a Source::TreeRewriter is empty?, i.e. if it has received any non-trivial command.

I wanted to use these methods to improve the autocorrection's API of rubocop, which could benefit from building a TreeRewriter, check if any correction was actually made, and combine them later in the process.

Copy link
Collaborator

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

Looks good overall, a few comments

lib/parser/source/tree_rewriter.rb Show resolved Hide resolved
lib/parser/source/tree_rewriter/action.rb Outdated Show resolved Hide resolved
test/test_source_tree_rewriter.rb Outdated Show resolved Hide resolved
test/test_source_tree_rewriter.rb Show resolved Hide resolved
Copy link
Collaborator

@iliabylich iliabylich left a comment

Choose a reason for hiding this comment

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

LGTM!

@iliabylich iliabylich merged commit f780d9a into whitequark:master Apr 14, 2020
@iliabylich
Copy link
Collaborator

@marcandre Thanks. Do you need a release?

@marcandre
Copy link
Contributor Author

Thanks so much for the quick review and constructive comments. I must say it is particularly appreciated tonight 😄

A release would be appreciated but is not urgent.

Thanks again ❤️

@marcandre
Copy link
Contributor Author

Actually, might as well wait until I make a PR for Source::Location#eql?

@iliabylich
Copy link
Collaborator

Actually, might as well wait until I make a PR for Source::Location#eql?

Sure!

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.

None yet

2 participants