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

Rename Lint/AmbiguousOperatorPrecedence to Style/ExplicitOperatorPrecedence #11111

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

koic
Copy link
Member

@koic koic commented Oct 22, 2022

Follow up #10080 (comment).

This PR renames Lint/AmbiguousOperatorPrecedence cop from Style/ExplicitOperatorPrecedence.

The above link comment would make a lot of sense. And other feedback on the issue has also been commented the cop shouldn't be at a particularly high "severity" level like Lint. I think it could be considered disabled by default, but this PR moves to Style instead of Lint anyway.

I understand that it shouldn't breaking change until 2.0, but I believe this is worth the breaking change for early adopters using pending cop.

As a side note, I think this should not be enforced by source code, as it can be completed in the editor if desired.
https://twitter.com/kddnewton/status/1485712576076042241


Before submitting the PR make sure the following are checked:

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

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 23, 2022

I understand that it shouldn't breaking change until 2.0, but I believe this is worth the breaking change for early adopters using pending cop.

I'll have to think about this, but I'm open to the suggestion in general. Plus I think we should start focusing our attention on RuboCop 2.0 soon anyways, as a lot of breaking changes have piled up in the backlog.

I also think that it's important for to start working on the "cop squads" or however we decide to name them in the end, as I think that allowing people to run some smaller more focused squads will solve much of such issues anyways.

…recedence`

Follow up rubocop#10080 (comment).

This PR renames `Lint/AmbiguousOperatorPrecedence` cop from `Style/ExplicitOperatorPrecedence`.

The above link comment would make a lot of sense. And other feedback on the issue has also
been commented the cop shouldn't be at a particularly high "severity" level like `Lint`.
I think it could be considered disabled by default, but this PR moves to `Style` instead of `Lint` anyway.

I understand that it shouldn't breaking change until 2.0, but I believe this is worth the breaking change
for early adopters using pending cop.

As a side note, I think this should not be enforced by source code, as it can be completed
in the editor if desired.
https://twitter.com/kddnewton/status/1485712576076042241
@koic koic force-pushed the rename_lint_ambiguous_operator_precedence_to_style_explicit_operator_precedence branch from 255a363 to 00b9a33 Compare January 12, 2023 06:53
@pirj
Copy link
Member

pirj commented Jan 12, 2023

What I'm most interested regarding the inheritance approach is if would offences be displayed for both the old and the new cop?

It was one of the decision-making factors for rubocop-capybara extraction to avoid duplicates, see this.

@koic
Copy link
Member Author

koic commented Jan 13, 2023

Ah, Indeed. Redundant warnings can be confusing. I'll take a closer look later.

@pirj
Copy link
Member

pirj commented Jan 13, 2023

Just to avoid any confusion, redundant offences, not warnings.
This is caused by both cops being enlisted.

$ rubocop spec/cap_spec.rb
Inspecting 1 file
C

Offenses:

spec/cap_spec.rb:3:5: C: Capybara/SpecificActions: Prefer click_link over find('a').click.
    find('a[href]').click
    ^^^^^^^^^^^^^^^^^^^^^
spec/cap_spec.rb:3:5: C: RSpec/Capybara/SpecificActions: Prefer click_link over find('a').click.
    find('a[href]').click
    ^^^^^^^^^^^^^^^^^^^^^

Contrary to the inheritance approach, rename in obsoletion config tells RuboCop to fall back from the old cop to the new one, and there is no redundancy in the output. At a cost of some extra code for cops doc generation.

@koic
Copy link
Member Author

koic commented Jan 13, 2023

Yeah. It's offense, not warning. I was using the wrong terminology 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants