-
-
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
Add new Lint/OutOfRangeRefInRegexp cop #7755 #8407
Add new Lint/OutOfRangeRefInRegexp cop #7755 #8407
Conversation
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.
Interesting idea :-)
I think this cop can only marked as unsafe though, as I don't think there's a way to be sure of the diagnostic.
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.
Pretty much done. Must be rebased to resolve conflict in Changelog.
Naming: OutOfRangeRefInRegexp
=> OutOfRangeRegexpRef
?
@@ -1,4 +1,4 @@ | |||
= Installation | |||
= Installation |
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.
You probably didn't mean to change this
config/default.yml
Outdated
@@ -1620,6 +1620,12 @@ Lint/OrderedMagicComments: | |||
Enabled: true | |||
VersionAdded: '0.53' | |||
|
|||
Lint/OutOfRangeRefInRegexp: | |||
Description: 'Checks for out of range reference for Regep because it always returns nil.' |
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.
Regep => Regexp
d9c62a4
to
a3497ff
Compare
@marcandre, I have resolved all the suggested changes, please have a look and let me know. |
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'm a tough reviewer, I hope you don't mind 😅.
There should be a test for encountering $4
before any regexp. You'll want to do an initialization in on_new_investigation
MSG = 'Do not use out of range reference for the Regexp.' | ||
|
||
def on_regexp(node) | ||
@valid_ref = cop_config['Count'] |
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'm not sure I understand cop_config['Count']
, maybe you simply meant nil
?
named_capture += 1 if e.instance_of?(Regexp::Expression::Group::Named) | ||
numbered_capture += 1 if e.instance_of?(Regexp::Expression::Group::Capture) |
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.
Is it possible to rely on e.type
instead of the class?
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, we can do it but we have to check with the token instead.
tree.each_expression do |e|
named_capture += 1 if e.token == :named
numbered_capture += 1 if e.token == :capture
end
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.
Neither seem great to me, but that might be the gem's fault. Could you use e.capturing?
, and then e.respond_to?(:name)
, say?
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 can check with e.capturing?
but again we cant check further for e.respond_to?(:name)
.
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.
Sorry, why?
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.
@marcandre, e.capturing?
will raise an error
undefined method capturing? for #<Regexp::Expression::CharacterType::Word:0x00007fb466af9cf0>
for all non-group type expressions so better we to use e.type?(:group)
then e.respond_to?(:name)
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.
e.type?(:group) && e.respond_to?(:name)
👍, I think it's much better than relying on the class, even if longer...
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'm a tough reviewer, and I don't see everything from the get go, I hope you don't mind 😅
You should add a test if $4
is encountered before any regexp. You'll need define on_new_investigation
too.
Never mind @marcandre, for me, it's a huge learning opportunity 😄 What I understood from the below comment
Do I need to initialize @valid_ref to nil in the on_new_investigation method? if so then I am not able to understand the use case for the on_new_investigation method because we are not creating any offense if we got the reference before any regexp. |
Yes
First, shouldn't we create an offense then? |
Please suggest we should consider this case to raise offense.
I got this one. Thanks 👍 |
Sorry, I don't understand... If I'm not mistaken, |
Yes, you are right. only for regular expression with non-literals, I have to make @valid_ref set to |
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.
Great! The implementation looks really good to me 🎉
You're still missing a test for $3
without a regexp though...
it 'does not register an offence when containing a ivar' do | ||
expect_no_offenses(<<~'RUBY') | ||
@var = '(\d+)' | ||
/(?<foo>#{@var}*)/ =~ "12" | ||
puts $1 | ||
puts $3 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offence when containing a cvar' do | ||
expect_no_offenses(<<~'RUBY') | ||
@@var = '(\d+)' | ||
/(?<foo>#{@@var}*)/ =~ "12" | ||
puts $1 | ||
puts $4 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offence when containing a gvar' do | ||
expect_no_offenses(<<~'RUBY') | ||
$var = '(\d+)' | ||
/(?<foo>#{$var}*)/ =~ "12" | ||
puts $1 | ||
puts $2 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offence when containing a method' do | ||
expect_no_offenses(<<~'RUBY') | ||
def do_something | ||
'(\d+)' | ||
end | ||
/(?<foo>#{do_something}*)/ =~ "12" | ||
puts $1 | ||
puts $4 | ||
RUBY | ||
end | ||
|
||
it 'does not register an offence when containing a constant' do | ||
expect_no_offenses(<<~'RUBY') | ||
CONST = "12" | ||
/(?<foo>#{CONST}*)/ =~ "12" | ||
puts $1 | ||
puts $3 | ||
RUBY | ||
end |
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.
All these tests can be removed, we only need to test one "dynamic" case.
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.
hi @marcandre but actually these specs are self-explanatory and devs who are reading these will get to understand the code better is what I think, please give an example of how this can be improved better with dynamic
test case.
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.
What I meant to say is that the previous test (with a variable) is sufficient; a good implementation (like we have now) can not pass the variable test and fail the instance variable, the method call, etc... or vice versa. They all test the same idea:is the regexp dynamic or not. It's not about what is inside the #{...}
, just that there is a #{...}
or not. Otherwise to be "complete", we'd have to add all possibilities, for example method call with block, or with rescue, or literal, or ...
it 'does not register offense when using a Regexp cannot be processed by regexp_parser gem' do | ||
expect_no_offenses(<<~'RUBY') | ||
/data = ({"words":.+}}}[^}]*})/m | ||
RUBY |
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.
Might as well add $3
or something. Hopefully the Regexp parser will handle this one day and that test will fail, but right now it would always pass.
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 still no reason to raise an offense here once parsing is fixed, right? That's why I wrote to use $3
or something.
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 have added $1 in last commit to fix this. please refer this
@marcandre, Please review the updated changes. let me know all good or we can still improve 😄 |
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.
Believe it or not, I found something else 🤣
module Lint | ||
# This cops looks for out of range referencing for Regexp, as while capturing groups out of | ||
# out of range reference always returns nil. | ||
|
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 just noticed that the doc file is incomplete, because you have a missing #
here
|
||
# @example | ||
# /(foo)bar/ =~ 'foobar' | ||
|
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.
and here...
|
||
# # bad - always returns nil | ||
# puts $2 # => nil | ||
|
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.
and here. Might be a cop idea! 😆
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.
Cool, hope I fixed everything now 🤣 🤞
9d91d81
to
c33997b
Compare
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.
Last tweaks, I promise 😅
it 'does not register offense when using a Regexp cannot be processed by regexp_parser gem' do | ||
expect_no_offenses(<<~'RUBY') | ||
/data = ({"words":.+}}}[^}]*})/m | ||
RUBY |
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 still no reason to raise an offense here once parsing is fixed, right? That's why I wrote to use $3
or something.
module RuboCop | ||
module Cop | ||
module Lint | ||
# This cops looks for out of range referencing for Regexp, as while capturing groups out of |
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'm not a native speaker, but the "as while" sounds strange to me. How about?
"This cops looks for references of Regexp captures that are out of range and thus always returns nil."
I have added $1 in the last commit to fix this. please refer this |
Right, but the "unparseable" regexp has one captured group, so |
Sorry, I missed that one, when I tried to use other than $1 i.e $2 or $3 this test is failing. I checked more on this and conclude that regex_parser now able to parse this type of expression.
Please suggest should we remove the rescue block then?
as Errors like this (https://github.com/ammar/regexp_parser/blob/master/spec/scanner/errors_spec.rb) are handled by Lint/Syntax cop.
|
That's good news :-) Right, I see in their Changelog the issue has been changed in v1.7.1, so we could bump our gemspec requirement to that version and remove the It also means that the other |
@marcandre Well, I got this ok I have to remove the |
No, rest can be done separately. You'll have to rebase to fix the Changelog conflict though |
8c36fe8
to
b5e412a
Compare
@marcandre, I have done rebase and all the changes. |
🎉 Thank you @sonalinavlakhe for this PR and your quick turn around to my multiple review comments 👍 Congratulations on your first Cop 🍾 😄 |
This cop looks for out of range referencing for Regexp, as while capturing groups out of range reference always returns nil.
Issue: #7755
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.