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

Allow specifying the location to report on for check_code_length #12676

Closed
G-Rath opened this issue Feb 5, 2024 · 2 comments · Fixed by #12814
Closed

Allow specifying the location to report on for check_code_length #12676

G-Rath opened this issue Feb 5, 2024 · 2 comments · Fixed by #12814

Comments

@G-Rath
Copy link

G-Rath commented Feb 5, 2024

Is your feature request related to a problem? Please describe.

"I'm always frustrated when..." I'm working on a complex test and suddenly my IDE flags every single line in the block because it's too long:

image

In the above, the first test is being flagged by RSpec/MultipleExpectations while the second is being flagged by RSpec/ExampleLength - while these cops belong to rubocop-rspec, the latter is literally just calling check_code_length which is a rubocop core method hence I'm opening this issue.

Describe the solution you'd like

I'd like RSpec/ExampleLength to report on the sending node rather than the whole block, so by extension I'd like to be able to somehow control the node check_code_length reports as the location of the offense.

Describe alternatives you've considered

Forking check_code_length in rubocop-rspec - there's not that much code so this doesn't seem like the worst thing in the world, but I expect it would be harder to convince the team to accept such a PR and really this issue could apply to any cop using check_code_length.

I could also disable the cop on the test while I'm working, but that's a bit annoying to have to manage - I've got to do it each time, and then remove it if I manage to shrink the example again.

Additional context

I've got three ideas on how this could be supported without a breaking change:

  1. support taking a block which would get called instead of add_offense, and passed the values currently being passed to add_offense; this would allow the most flexibility
    • (I think this is probably the best way to go since its not an error to pass a block to a method that doesn't yield so it'd gracefully fallback on older versions of Rubocop)
  2. make location a param with a default value; I think that should be completely not breaking, though it would be annoying having such a complex default value and technically the method would do a bit more work than it does now when node.line_count <= max_length
  3. make an internal check_code_length and then expose a couple of public methods with different signatures (including the current "original", + check_code_length_with_location etc); effectively 2. but in a way that makes it easier to avoid breaking changes if the signature needs to change in future
@G-Rath
Copy link
Author

G-Rath commented Feb 9, 2024

@koic fwiw I'm happy to have a crack at this, I just would like to know if maintainers are ok with what I've suggested or have other ideas or etc 🙂

@G-Rath
Copy link
Author

G-Rath commented Mar 3, 2024

So I'm looking into this, and not sure if my preferred "pass a block" idea will work because a block is already being passed to add_offense though I'm not sure what it's doing:

add_offense(location, message: message(length, max_length)) { self.max = length }

From what I understand the block is for autocorrection, but this isn't doing anything autocorrecting-ish - instead its doing an assignment, which looks to end up calling a dynamically created method via RuboCop::ExcludeLimit#exclude_limit that indicates it's about setting values up for --auto-gen-config...

Meanwhile I've also just found #2414 and #2629 which might indicate the current reporting location was intentionally switched to - I suspect that the ecosystem has since changed in a manner that makes it fine to revisit but I'm overall getting the feeling this might be harder to tackle than initially thought.

Any guidance or opinions would be welcome :)

koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 1, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
bbatsov pushed a commit that referenced this issue Apr 2, 2024
Fixes #12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
koic added a commit to koic/rubocop that referenced this issue Apr 7, 2024
Fixes rubocop#12676.

Highlighting offenses for entire classes, modules, and methods is considered noisy in LSP.
Instead, highlighting will be applied only to the class names or method names for offenses.

To maintain compatibility for uses outside of LSP, such as with `--disable-uncorrectable`,
the behavior will be changed only when `RuboCop::LSP.enabled?` is true.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants