-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Handle comments and blank interpolations in regexp parsed_tree #8803
Handle comments and blank interpolations in regexp parsed_tree #8803
Conversation
s.add_runtime_dependency('rexml') | ||
s.add_runtime_dependency('rubocop-ast', '>= 0.5.0') | ||
s.add_runtime_dependency('rubocop-ast', '>= 0.6.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pick up rubocop/rubocop-ast#124
@@ -36,9 +36,9 @@ Gem::Specification.new do |s| | |||
s.add_runtime_dependency('parallel', '~> 1.10') | |||
s.add_runtime_dependency('parser', '>= 2.7.1.5') | |||
s.add_runtime_dependency('rainbow', '>= 2.2.2', '< 4.0') | |||
s.add_runtime_dependency('regexp_parser', '>= 1.7') | |||
s.add_runtime_dependency('regexp_parser', '>= 1.8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pick up ammar/regexp_parser#68 and ammar/regexp_parser#67
cb668c1
to
7693728
Compare
CHANGELOG.md
Outdated
@@ -2,6 +2,10 @@ | |||
|
|||
## master (unreleased) | |||
|
|||
### Changes | |||
|
|||
* [#8803](https://github.com/rubocop-hq/rubocop/pull/8803): Extend RegexpNode#parsed_tree to handle regexps including interpolaton and comments. ([@owst][]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^interpolaton^interpolation
Also, please back-quote RegexpNode#parsed_tree
Might change altogether, read below
lib/rubocop/ext/regexp_node.rb
Outdated
def parsed_tree | ||
return if interpolation? | ||
def parsed_tree(interpolation: :ignore) | ||
unless %i[ignore blank].include?(interpolation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When thinking about the fact that :ignore
is confusing (it's "ignore the regexp itself", not the interpolations), the only good name I could come up with was :return_nil
and that's a code smell.
That made me wonder if the best course of action isn't to remove the option altogether, and make blanking the default. What do you think? It should be the callers job to check for interpolation lint/mixed_regexp_capture_types
would need to do that).
Should be documented, and marked as breaking change in the Changelog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'm not really a fan of interpolation :ignore
. I like the idea of always blanking (that is, until we find a scenario where it isn't appropriate 😄).
Having had a quick look at lint/mixed_regexp_capture_types
I think it could also just use blanking, as we could catch strictly more issues (where the regexp contains mixed capture groups in addition to an interpolation)
I'll take a look at making this change, and report back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right about lint/mixed_regexp_capture_types
🤦♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for now, to keep this PR focussed, I left lint/mixed_regexp_capture_types
as is - i.e. it still does nothing in the presence of interpolation. A nice follow-up might be to change it, as I mentioned previously
lib/rubocop/ext/regexp_node.rb
Outdated
private | ||
|
||
def with_interpolations_blanked | ||
children.reject(&:regopt_type?).map do |child| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only one regopt
at the end, right? If so, .reject(&:regopt_type?)
=> [0...-1]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I don't think I'd realised that even for a Regexp without options the regopt node was still present. I think the original was better readability-wise, but the new version is probably fine with a comment 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about I add a RegexpNode#content_nodes
to rubocop-ast
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, my slight concern is that RegexpNode#content
already exists and ignores interpolation (those with begin_type?
) nodes, whereas RegexpNode#content_nodes
would not ignore interpolation nodes. I'm not sure I have an alternative suggestion, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know. I would actually mark content
as # @deprecated
; I can't think of a single good use for that method.
…nd_blank_interpolations_in_regexp_parsed_tree
…ments_and_blank_interpolations_in_regexp_parsed_tree
…nd_blank_interpolations_in_regexp_parsed_tree
…nd_blank_interpolations_in_regexp_parsed_tree
…ments_and_blank_interpolations_in_regexp_parsed_tree
Great stuff, thanks! |
Pulling out a PR to add this functionality, as mentioned by @marcandre: #8625 (comment)
Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.