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

Support Array.new(n) on RSpec/FactoryBot/CreateList cop #1373

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

r7kamura
Copy link
Contributor

rubocop-performance's Performance/TimesMap cop autocorrects n.times into Array.new(n), so it would be nice if this cop supports autocorrection from Array.new(n) { create(:a) } to create_list(:a, n).


Before submitting the PR make sure the following are checked:

  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Updated documentation.
  • Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
  • The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

  • Added the new cop to config/default.yml.
  • The cop is configured as Enabled: pending in config/default.yml.
  • The cop is configured as Enabled: true in .rubocop.yml.
  • The cop documents examples of good and bad code.
  • The tests assert both that bad code is reported and that good code is not reported.
  • Set VersionAdded in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

  • Set VersionChanged in config/default.yml to the next major version.

@@ -57,6 +57,31 @@ class CreateList < Base
)
PATTERN

# @!method array_new_block?(node)
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can merge the patterns, as they differ by the receiver

if node.receiver.int_type?
node.receiver
else
node.send_node.arguments.first
Copy link
Member

Choose a reason for hiding this comment

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

there's also first_argument method for send nodes ;)

@r7kamura r7kamura force-pushed the feature/array-new branch 2 times, most recently from 84b0b89 to 7d8f547 Compare August 30, 2022 21:21
@r7kamura r7kamura requested a review from Darhazer August 30, 2022 21:24
# @!method n_times_block?(node)
def_node_matcher :n_times_block?, <<-PATTERN
# @!method array_new_or_n_times_block?(node)
def_node_matcher :array_new_or_n_times_block?, <<-PATTERN
Copy link
Member

Choose a reason for hiding this comment

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

I'd use some more generic name for this, like correctable_block? but I leave this up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought of that kind of name once myself, but I don't think it is necessarily correctable only by this condition.

If we create a method called #correctable_block?, the implementation should look something like this:

def correctable_block?(node)
  style == :create_list &&
    array_new_or_n_times_block?(node) &&
    node.body &&
    !arguments_include_method_call?(node.body) &&
    contains_only_factory?(node.body)
end

Copy link
Collaborator

@bquorning bquorning Sep 10, 2022

Choose a reason for hiding this comment

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

I think I learned once that method names containing "or" or "and" should be avoided, and implemented as two methods instead. Would there be significant impact (readability, performance or other) if we chose to keep #n_times_block? and added an #array_new_block? matcher?

By the way, this doesn’t mean I think that block_with_arg_and_used? should be split 😄 But perhaps it could use another name?

Copy link
Member

@Darhazer Darhazer Sep 10, 2022

Choose a reason for hiding this comment

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

Originally they were split, but since were so similar, I suggested having just one. We haven’t come up with a good name though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, sorry about that. I really should look at the previous conversations before I comment 😅

@ydah
Copy link
Member

ydah commented Sep 5, 2022

Perhaps, but it seems that adding an entry to CHANGELOG.md is not included in the pull request.
Please add an entry to the CHANGELOG.md .

@r7kamura r7kamura force-pushed the feature/array-new branch 2 times, most recently from 086128c to af6397a Compare September 5, 2022 21:19
@r7kamura
Copy link
Contributor Author

r7kamura commented Sep 5, 2022

I added a missing entry to CHANGELOG.md. Thanks!

Comment on lines 220 to 224
if node.receiver.int_type?
node.receiver
else
node.send_node.first_argument
end.source
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we give a name to the thing we call source on? That might allow me to think a bit less about what the method does 😅

Suggested change
if node.receiver.int_type?
node.receiver
else
node.send_node.first_argument
end.source
foo = if node.receiver.int_type?
node.receiver
else
node.send_node.first_argument
end
foo.source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll use the name count_node for this variable 👍

  3.times
# ^ this node

  Array.new(3)
          # ^ or this node

Copy link
Collaborator

@bquorning bquorning left a comment

Choose a reason for hiding this comment

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

@Darhazer Feel free to merge if you’re still happy with the changes.

@Darhazer Darhazer merged commit 71a3efb into rubocop:master Sep 12, 2022
@r7kamura r7kamura deleted the feature/array-new branch September 12, 2022 20:03
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

4 participants