Implement Style/SlicingWithRange cop #7921

zverok · 2020-04-30T12:34:39Z

Under Ruby >= 2.6, proposes to change ary[x..-1] to ary[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:

ary[1..-1]
    ^^^^^ When slicing array till its end, prefer endless range: ary[n..] instead of ary[n..-1].

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
~~[ ] Commit message starts with [Fix #issue-number] (if the related issue exists).~~
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

rrosenblum · 2020-05-01T16:27:09Z

lib/rubocop/cop/style/endless_range.rb

+        def_node_matcher :range_till_minus_one?, '(irange (int _) (int -1))'
+
+        def on_send(node)
+          return unless node.method?(:[]) && node.arguments.count == 1


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)))

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.

rrosenblum · 2020-05-01T16:30:32Z

spec/rubocop/cop/style/endless_range_spec.rb

+      RUBY
+    end
+
+    it 'autocorrects' do


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

Oh, cool, thanks. Fixed.

bbatsov · 2020-05-08T19:21:25Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### New features

+* Add [Style/EndlessRange] cop. ([@zverok][])


I guess you can link to your PR here.

bbatsov · 2020-05-08T19:23:25Z

UPD: Looked at it with fresh eyes, maybe name is too vague? Like SliceWithEndlessRange would be better?

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.

bbatsov · 2020-05-08T19:24:45Z

lib/rubocop/cop/style/endless_range.rb

+
+        minimum_target_ruby_version 2.6
+
+        MSG = 'Prefer endless ranges (start..) for slicing '\


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.

bbatsov · 2020-05-08T19:26:18Z

manual/cops_style.md

@@ -1996,6 +1996,25 @@ at_exit { puts 'Goodbye!' }

 * [https://rubystyle.guide#no-END-blocks](https://rubystyle.guide#no-END-blocks)

+## Style/EndlessRange


@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.

This idea looks good to me! So, each Ruby version supports new syntax and APIs. It can be a work independent of this PR :-)

Yeah, totally. I just thought of this while going over this PR and thought I should share my thoughts with you.

zverok · 2020-05-09T12:50:56Z

@bbatsov @koic I've updated the PR according to comments (no "version in docs"/"update to styleguide" yet, tho). Can you please take a look?

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`.

bbatsov · 2020-05-10T05:48:38Z

lib/rubocop/cop/style/slicing_with_range.rb

+
+        minimum_target_ruby_version 2.6
+
+        MSG = 'When slicing array till its end, prefer endless range: '\


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.

bbatsov · 2020-05-10T05:49:05Z

lib/rubocop/cop/style/slicing_with_range.rb

+        minimum_target_ruby_version 2.6
+
+        MSG = 'When slicing array till its end, prefer endless range: '\
+              'ary[n..] instead of ary[n..-1].'


Ideally here we should use the actual array name from the code, but that's not a big deal.

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.

bbatsov · 2020-05-10T05:49:29Z

lib/rubocop/cop/style/slicing_with_range.rb

+  module Cop
+    module Style
+      # This cop checks that arrays are sliced with endless ranges instead of
+      # `ary[start..-1]` on Ruby 2.6+


This sentence should end with a ..

bbatsov · 2020-05-10T05:51:16Z

Looks good to me overall. I've added a few tiny remarks, though. Btw, rebase on top of master and re-run the docs generator and you'll get the required Ruby version in the docs.

Under Ruby >= 2.6, proposes to change ary[x..-1] to ary[x..]. Supports autocorrect.

zverok · 2020-05-10T07:53:46Z

@bbatsov All fixed.
(About error messages — I thought a bit about their "teaching/explanatory" value, looking at Elixir's credo, but that's a large topic unrelated to this PR.)

bbatsov · 2020-05-10T09:43:31Z

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.

zhuravel · 2020-05-12T21:46:54Z

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]

zverok · 2020-05-12T22:04:25Z

@zhuravel thanks, I'll investigate

Follow rubocop#7921. This PR marks `Style/SlicingWithRange` as safe.

rrosenblum reviewed May 1, 2020

View reviewed changes

zverok force-pushed the endless_range_cop branch from 96f24f5 to ad87ac5 Compare May 3, 2020 18:20

bbatsov reviewed May 8, 2020

View reviewed changes

zverok changed the title ~~Implement Style/EndlessRange cop~~ Implement Style/SlicingWithRange cop May 9, 2020

zverok force-pushed the endless_range_cop branch from 29074d6 to 2c50eeb Compare May 9, 2020 12:01

koic mentioned this pull request May 9, 2020

Add required Ruby version to generated cops manual #7947

Merged

8 tasks

koic added a commit to koic/rubocop that referenced this pull request May 9, 2020

Add required Ruby version to generated cops manual

a7e3299

Follow up to rubocop#7921 (comment). This PR adds required Ruby version to cops manual generated by `rake generate_cops_documentation`.

bbatsov pushed a commit that referenced this pull request May 10, 2020

Add required Ruby version to generated cops manual

4a41a06

Follow up to #7921 (comment). This PR adds required Ruby version to cops manual generated by `rake generate_cops_documentation`.

bbatsov reviewed May 10, 2020

View reviewed changes

Implement Style/SlicingWithRange cop

b62ccfc

Under Ruby >= 2.6, proposes to change ary[x..-1] to ary[x..]. Supports autocorrect.

zverok force-pushed the endless_range_cop branch from 2c50eeb to b62ccfc Compare May 10, 2020 07:51

bbatsov merged commit 0c3d278 into rubocop:master May 10, 2020

zverok deleted the endless_range_cop branch May 10, 2020 11:57

zverok mentioned this pull request May 13, 2020

More powerful SlicingWithRange #7967

Merged

7 tasks

searls mentioned this pull request May 18, 2020

Should we add Style/SlicingWithRange standardrb/standard#175

Closed

koic mentioned this pull request Oct 13, 2020

Mark Style/SlicingWithRange as safe #8890

Closed

8 tasks

koic added a commit to koic/rubocop that referenced this pull request Oct 15, 2020

Mark Style/SlicingWithRange as safe

89cd0e3

Follow rubocop#7921. This PR marks `Style/SlicingWithRange` as safe.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Style/SlicingWithRange cop #7921

Implement Style/SlicingWithRange cop #7921

zverok commented Apr 30, 2020 •

edited

rrosenblum May 1, 2020

zverok May 3, 2020

rrosenblum May 1, 2020

zverok May 3, 2020

bbatsov May 8, 2020

bbatsov commented May 8, 2020

bbatsov May 8, 2020

bbatsov May 8, 2020

koic May 9, 2020

bbatsov May 9, 2020

zverok commented May 9, 2020

bbatsov May 10, 2020

bbatsov May 10, 2020

zverok May 10, 2020

bbatsov May 10, 2020

bbatsov commented May 10, 2020

zverok commented May 10, 2020

bbatsov commented May 10, 2020

zhuravel commented May 12, 2020

zverok commented May 12, 2020

		@@ -4,6 +4,7 @@

		### New features

		* Add [Style/EndlessRange] cop. ([@zverok][])


		minimum_target_ruby_version 2.6

		MSG = 'Prefer endless ranges (start..) for slicing '\

		@@ -1996,6 +1996,25 @@ at_exit { puts 'Goodbye!' }

		* [https://rubystyle.guide#no-END-blocks](https://rubystyle.guide#no-END-blocks)

		## Style/EndlessRange


		minimum_target_ruby_version 2.6

		MSG = 'When slicing array till its end, prefer endless range: '\

Implement Style/SlicingWithRange cop #7921

Implement Style/SlicingWithRange cop #7921

Conversation

zverok commented Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zverok commented May 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented May 10, 2020

zverok commented May 10, 2020

bbatsov commented May 10, 2020

zhuravel commented May 12, 2020

zverok commented May 12, 2020

zverok commented Apr 30, 2020 •

edited