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

Fix Style/BlockDelimiters for blocks with numbered arguments #10749

Merged
merged 1 commit into from Jun 26, 2022

Conversation

gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Jun 26, 2022

With EnforcedStyle of line_count_based, the default, the following code will issue an offense, and will be auto-corrected:

[1, 2, 3].map { |n| # [1, 2, 3].map do |n|
  n + 1             #   n + 1
}                   # end

While the code below won't be converted to a multiline do ... end block:

[1, 2, 3].map {
  _1 + 1
}

This is yet another case of missing on_numblock implementation. I see a dozen of those in the codebase and I think whenever we handle block AST nodes we should also handle numblock nodes. I can go over the codebase and fix those problems on a per-cop basis, or I can try to fix it generally.

Currently, my best idea is to write an InternalAffairs cop that warns when we define on_block handlers, but we miss an on_numblock implementation. For the majority of the cases, on_numblock alias of the on_block method would be enough to handle blocks with numbered arguments. I can then use the internal cop and systematically go over the cases and add aliases or custom numblock handlers where required. I think this is better than solutions involving meta-programming or default on_numblock implementations delegating to on_block.


  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • 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.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

With `EnforcedStyle` of `line_count_based`, the default, the following
code **will** issue an offense, and will be auto-corrected:

```ruby
[1, 2, 3].map { |n| # [1, 2, 3].map do |n|
  n + 1             #   n + 1
}                   # end

```

While the code below won't be converted to a multiline `do ... end`
block:

```ruby
[1, 2, 3].map {
  _1 + 1
}
```

This is yet another case of missing `on_numblock` implementation. I see
a dozen of those in the codebase and I think whenever we handle `block`
AST nodes we should also handle `numblock` nodes. I can go over the
codebase and fix those problems on a per-cop basis, or I can try to fix
it generally.

Currently, my best idea is to write an `InternalAffairs` cop that warns
when we define `on_block` handlers, but we miss a `on_block`
implementation. For the majority of the cases, an `on_numblock` alias of
the `on_block` method would be enough to handle blocks with numbered
arguments. I can then use the internal co and systematically go over the
cases and add aliases or custom `numblock` handlers where required. I
think this is better than solutions involving meta-programming or
default `on_numblock` implementations delegating to `on_block`.
@koic koic merged commit ae3dec3 into rubocop:master Jun 26, 2022
29 of 30 checks passed
@koic
Copy link
Member

@koic koic commented Jun 26, 2022

Thanks!

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

2 participants