Skip to content

Add RSpec/MemoizedHelperBlockDelimiter cop#1419

Closed
r7kamura wants to merge 1 commit intorubocop:masterfrom
r7kamura:feature/memoized-helper-block-delimiter
Closed

Add RSpec/MemoizedHelperBlockDelimiter cop#1419
r7kamura wants to merge 1 commit intorubocop:masterfrom
r7kamura:feature/memoized-helper-block-delimiter

Conversation

@r7kamura
Copy link
Copy Markdown
Contributor

@r7kamura r7kamura commented Oct 22, 2022

In response to the review comment at #1414 (comment), I made a cop to be consistent in the way memoized helpers such as let and subject are written.

My personal opinion is that since these helpers are method definitions, they should be written in a style similar to that of writing def. I don't see many cases where def is written in one-liner style. For that reason, I thought that I should prefer to use a consistent style regardless of whether the content of the block would be short or long, and that do_end should be the default.

There is another reason for wanting the default to be this way: the braces style may conflict with Style/BlockDelimiters cop in some cases, so it may not be possible to autocorrect to always use braces. I would also like to avoid the put another style to use different delimiters for different situations as possible because this increases the cognitive load.

I think this cop is somewhat opinionated, so I would like to hear your opinions first.


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.

Copy link
Copy Markdown
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

To be useful, I believe the cop should work together with Layout/LineLength somehow. If the style is braces, the helper should be inlined unless this would break Layout/LineLength.

The default style should be braces, and it shouldn't even raise an offence for do/end usages if this would break the line length constraint set by Layout/LineLength.

I believe it is possible to offer auto-correction for braces, too, as:

  1. It's not necessary that BlockDelimiters cop is enabled in the user configuration
  2. It is possible to inline (except when it breaks line length)

PS The cop should ignore multi-statement definitions for the braces style

# @param node [RuboCop::AST::BlockNode]
# @return [Boolean]
def memoized_helper_method?(node)
let?(node) || subject?(node)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will also match blockpass, should it?

Comment on lines +98 to +99
def memoized_helper_method?(node)
let?(node) || subject?(node)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suggest turning this into a node pattern:

        {
          #{block_pattern('#Helpers.all')}
          #{block_pattern('#Subjects.all')}
        }

# @param node [RuboCop::AST::BlockNode]
# @return [Boolean]
def bad?(node)
memoized_helper_method?(node) &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line doesn't semantically belong to "bad", it detects if it's a match.
I suggest moving it to on_block:

memoized_helper_method(node) do |memoized_helper|
  next if preferred_block_delimiter?(memoized_helper)
  add_offense(...)

Comment thread config/default.yml
RSpec/MemoizedHelperBlockDelimiter:
Description: Use consistent style block delimiters for memoized helpers.
Enabled: pending
EnforcedStyle: do_end
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I disagree with the choice of the default style.

do_end doesn't seem to be a frequent choice in the wild, according to a run against real-world-rspec:

57497 files inspected, 104170 offenses detected, 104170 offenses autocorrectable

as opposed to braces:

57497 files inspected, 17121 offenses detected

And, I suspect some part of do/end usages (offences to braces style) are due to the line length limitation:

    let!(:conference) do
      WimbaConference.create!(title: "my conference", user: @user, duration: 60, context: course_factory)
    end

or because of being multi-line e.g. /Users/pirj/source/real-world-rspec/spree/emails/spec/mailers/spree/order_mailer_spec.rb:25:22::

  let(:second_order) do
    order = create(:completed_order_with_totals, email: 'test2@example.com')
    product = create(:product, name: %{The "BESTEST" product})
    variant = create(:variant, product: product)
    price = create(:price, variant: variant, amount: 15.00)
    store = second_store
    line_item = create(:line_item, variant: variant, order: order, quantity: 1, price: 4.99)
    allow(product).to receive_messages(default_variant: variant)
    allow(variant).to receive_messages(default_price: price)
    allow(order).to receive_messages(line_items: [line_item])
    allow(order).to receive_messages(store: store)
    order
  end

@r7kamura
Copy link
Copy Markdown
Contributor Author

Sorry for the late reply.
It is true that the default should be braces, considering the usage ratio of existing code, and Layout/LineLength should be taken into account. I don't know if I can implement this well, but I will give it a try 👌

@r7kamura
Copy link
Copy Markdown
Contributor Author

While I was able to implement the do-end style well, I could not come up with a good implementation or appropriate rules for the braces style. I'll close this Pull Request for now 🙇

@r7kamura r7kamura closed this Nov 11, 2022
@r7kamura r7kamura deleted the feature/memoized-helper-block-delimiter branch November 11, 2022 23:18
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.

2 participants