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
Implement Style/SlicingWithRange cop #7921
Conversation
def_node_matcher :range_till_minus_one?, '(irange (int _) (int -1))' | ||
|
||
def on_send(node) | ||
return unless node.method?(:[]) && node.arguments.count == 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.
I think you could handle all of these checks with a single def_node_matcher
. It's been a little while since I wrote one, I think it would look like (send _ :[] (irange (int _) (int -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.
Of course I can, but I believe it is better this way for concern separation.
"Works only for method #[]
with a singular argument" is an arbitrary decision, which can be changed in future and/or depend on settings (for ex., one may also expect #slice
to be checked, or multi-argument #[]
when workin with matrices). But "range ending in -1
" is what the cop is about. So I believe it is better to leave as two separate checks.
RUBY | ||
end | ||
|
||
it 'autocorrects' do |
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 can handle auto-correction in the same method where you check for the offense by using expect_correction
.
expect_offense(<<~RUBY)
ary[1..-1]
^^^^^ Prefer endless ranges (start..) for slicing till the end of array.
RUBY
expect_correction(<<~RUBY)
ary[1..]
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.
Oh, cool, thanks. Fixed.
CHANGELOG.md
Outdated
@@ -4,6 +4,7 @@ | |||
|
|||
### New features | |||
|
|||
* Add [Style/EndlessRange] cop. ([@zverok][]) |
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 guess you can link to your PR here.
That's a good point. In general we should choose more neutral names and aim to support both styles, even if I agree what you suggested is the better style. I'd also suggest adding a guideline on the subject in the Ruby style guide. |
|
||
minimum_target_ruby_version 2.6 | ||
|
||
MSG = 'Prefer endless ranges (start..) for slicing '\ |
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 guess we can improve this message by inserting code snippets in it - e.g. prefer [new code] over [existing code]. I've noticed that people tend to understand such messages better.
manual/cops_style.md
Outdated
@@ -1996,6 +1996,25 @@ at_exit { puts 'Goodbye!' } | |||
|
|||
* [https://rubystyle.guide#no-END-blocks](https://rubystyle.guide#no-END-blocks) | |||
|
|||
## Style/EndlessRange |
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.
@koic I guess it won't be a bad idea to somehow incorporate the required Ruby version in this docs snippet. Might be a good idea to draw people's attention to the fact that some cops run only on certain Rubies.
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.
This idea looks good to me! So, each Ruby version supports new syntax and APIs. It can be a work independent of this PR :-)
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, totally. I just thought of this while going over this PR and thought I should share my thoughts with you.
Follow up to rubocop#7921 (comment). This PR adds required Ruby version to cops manual generated by `rake generate_cops_documentation`.
Follow up to #7921 (comment). This PR adds required Ruby version to cops manual generated by `rake generate_cops_documentation`.
|
||
minimum_target_ruby_version 2.6 | ||
|
||
MSG = 'When slicing array till its end, prefer endless range: '\ |
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 can just shorten this to Prefer x over y
, as I think the rest is implied anyways. Plus, this way it's going to be more consistent with the rest of the messages used by RuboCop.
minimum_target_ruby_version 2.6 | ||
|
||
MSG = 'When slicing array till its end, prefer endless range: '\ | ||
'ary[n..] instead of ary[n..-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.
Ideally here we should use the actual array name from the code, but that's not a big deal.
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.
Thought about it but decided against it: realistic case may look like chain.of(...).lot...of.operations(...)[calculating.the.first(...).index..-1]
, and trying to reproduce the whole statement in cop's message will blur the intention.
module Cop | ||
module Style | ||
# This cop checks that arrays are sliced with endless ranges instead of | ||
# `ary[start..-1]` on Ruby 2.6+ |
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.
This sentence should end with a .
.
Looks good to me overall. I've added a few tiny remarks, though. Btw, rebase on top of |
Under Ruby >= 2.6, proposes to change ary[x..-1] to ary[x..]. Supports autocorrect.
Thanks, Victor! Yeah, the messages are a whole different topic indeed. I'm all for descriptive messages, but there's also the matter that those are used in many different context (not just in the terminal) and longer messages are problematic in some of those. That's why I also like to point people for details to the style guide, as there it's much easier to have detailed explanations. |
Sample lines for which this cop does not generate warnings: integer, fractional = number[0 ... most_right_index], number[most_right_index.next .. -1]
integer, fractional = number[0...most_right_index], number[most_right_index.next..-1]
integer, fractional, delimiter = number[0 ... i], number[i.next .. -1], number[i] |
@zhuravel thanks, I'll investigate |
Follow rubocop#7921. This PR marks `Style/SlicingWithRange` as safe.
Under Ruby >= 2.6, proposes to change
ary[x..-1]
toary[x..]
. This is what endless ranges were introduced for, and it is more obvious than a (slightly weird) agreement of "-1
is the end".Works only for integer
x
and only for one-argument[]
method; correctly ignores exclude-end ranges.Supports autocorrect.
Example of output:
Before submitting the PR make sure the following are checked:
[ ] Commit message starts with[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.