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

Avoid annoyance from new rules #448

Open
straight-shoota opened this issue Jan 15, 2024 · 4 comments
Open

Avoid annoyance from new rules #448

straight-shoota opened this issue Jan 15, 2024 · 4 comments
Labels

Comments

@straight-shoota
Copy link
Contributor

straight-shoota commented Jan 15, 2024

Quoting @Blacksmoke16 from #447 (comment)

It just feels less than ideal that new rules keep being added that cannot be actually robust without semantic analysis, or are more subjective in general. Which ends up forcing me to either disable them inline, globally, or conform to the suggestions on every minor release.

I still think some/most of these should be disabled by default, or made optional via some sort of opinionated style #112.

I think this is quite similar to the considerations about the Crystal formatter rolling out new formats (c.f. crystal-lang/crystal#13002).
The formatter is tied to Crystal releases. Ameba on the other hand has more freedom about chosing when to release new rules.
How exactly that can be utilized, is up for discussion.

But in general, I think it would be very helpful to avoid required user interaction every time a new rule is added. That's especially if the rule has a high rate of false positives (such as Lint/UselessAssign). That just keeps annoying users.

Maybe for a project with existing ameba configuration, all rules that have been added since the version that generated the ameba config could be disabled by default? I.e. if I have a config from ameba 1.6, ameba will only consider rules from 1.6 until I explicitly upgrade to a new version and ruleset.

@Sija
Copy link
Member

Sija commented Jan 18, 2024

But in general, I think it would be very helpful to avoid required user interaction every time a new rule is added. That's especially if the rule has a high rate of false positives (such as Lint/UselessAssign). That just keeps annoying users.

I disagree, this argument has been based on a false assertion of high false positive rate of UselessAssign rule. Yes, there were several, due to the recent changes, yet this doesn't constitute this rule as having "high rate of false positives".

Maybe for a project with existing ameba configuration, all rules that have been added since the version that generated the ameba config could be disabled by default? I.e. if I have a config from ameba 1.6, ameba will only consider rules from 1.6 until I explicitly upgrade to a new version and ruleset.

This would mean an extra DX hassle which I'd like to avoid. Let's think about this more.

@Sija Sija added the question label Jan 18, 2024
@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Jan 18, 2024

I disagree, this argument has been based on a false assertion of high false positive rate of UselessAssign rule.

Not really, this is just the latest occurrence. In the past there was Lint/NotNil, and the change in all the Naming/* rules. It just feels like a lot of the rules are becoming more subjective. Plus with them being enabled by default, it forces the users to conform, or disable the rules.

I don't have any strong feelings around a way forward, but one idea that comes to mind is introducing new rules, keeping them disabled, then at some point in the future, do a new major release that changes the default rules.

EDIT: Another idea is having some sort of tiered rule system. Where tier 1 can be the most useful objective rules, then tier 2 gets a bit less objective, but still conforms to convention, and tier 3+ are Ameba's opinionated rules.

@snacks02
Copy link

snacks02 commented Jan 31, 2024

I would note that I'm having the same issue.

Though I think it predates Ameba. Here is what is disabled in .rubocop.yml in one of my Ruby projects:

Metrics/AbcSize:
  Enabled: false

Metrics/BlockLength:
  Enabled: false

Metrics/ClassLength:
  Enabled: false

Metrics/CyclomaticComplexity:
  Enabled: false

Metrics/MethodLength:
  Enabled: false

Metrics/ModuleLength:
  Enabled: false

Metrics/ParameterLists:
  Enabled: false

Metrics/PerceivedComplexity:
  Enabled: false

Minitest/MultipleAssertions:
  Enabled: false

Rails/CreateTableWithTimestamps:
  Enabled: false

Rails/DangerousColumnNames:
  Enabled: false

Rails/FindEach:
  Enabled: false

Rails/I18nLocaleTexts:
  Enabled: false

Rails/Pluck:
  Enabled: false

Style/Documentation:
  Enabled: false

Style/EmptyCaseCondition:
  Enabled: false

I often have to disable a couple new rules when updating RuboCop.

Here's what .ameba.yml looks like in one of my projects:

Documentation/DocumentationAdmonition:
  Enabled: false

Lint/UselessAssign:
  Enabled: false

Metrics/CyclomaticComplexity:
  Enabled: false

Naming/BlockParameterName:
  Enabled: false

Three of these were added in the latest minor release, with Lint/UselessAssign having a false positive here (see IHDR usage):

@Sija
Copy link
Member

Sija commented Apr 3, 2024

@sanks02 False positive you've mentioned was fixed in #443

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

No branches or pull requests

4 participants