Skip to content

Add unnecessary_underscores to core/recommended set. #856

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

Closed
kallentu opened this issue Feb 3, 2025 · 8 comments
Closed

Add unnecessary_underscores to core/recommended set. #856

kallentu opened this issue Feb 3, 2025 · 8 comments

Comments

@kallentu
Copy link
Member

kallentu commented Feb 3, 2025

Describe the rule you'd like to see added and to what rule set

https://dart.dev/tools/linter-rules/unnecessary_underscores

When a user is using multiple underscores (__) and that variable isn't referenced, they should use a wildcard variable _ instead.

Enabling this lint will allow us to avoid that pattern which is also the point of having the wildcard variables feature 😀

This should at least be in the recommended lint set.
I'd love to see it in the core lint set though.

We should be encouraging users to use wildcard variables where they can and avoid multiple underscores all together. Wildcard variables provide static errors when used and would prevent errors like referencing an intentionally unused parameter by accident.

Additional context

See paragraph above.

@natebosch
Copy link
Member

I'm in favor. It's mostly a style fix but I'm inclined to put it in the core set.

@lrhn
Copy link
Member

lrhn commented Feb 5, 2025

I'm not sure it warrantes being in core.
It's not any kind of warning sign that code contains __, it's just old code that still works just as well.
It's unlikely to cause an error, it's likely not in public API. It's a small implementation detail that there is no reason to ding people points for on Pub.dev.

@natebosch
Copy link
Member

natebosch commented Feb 5, 2025

it's likely not in public API.

For public API this might warrant inclusion in the core set, but you're right that it will be incredibly rare. It's definitely not worth splitting the implementation just to be able to include it in the core set.

no reason to ding people points for on Pub.dev.

There's also no (good) reason for maintainers of published packages who are anyways upgrading their language version to not apply the quick fixes for this lint.

I'd be happy enough to see this in the recommended set, but I do think it is likely to do more good (alert maintainers of a readability improvement they may have overlooked) than harm (annoy maintainers of published packages who prefer ugly code to any amount of churn?) if we put it in core.

@lrhn
Copy link
Member

lrhn commented Feb 6, 2025

The lint should also only apply to code that is 3.7 or above (that's where we released it, right?), so if you're not touching your old code, you won't lose points.
If you move your code base to 3.7, you'll get the warning from a recommended lint anyway and will likely just fix it.

I don't think it'll matter much for adaption whether the lint is in core or not. I expect authors that edit their code to use the recommended lints.
I do prefer to only put lints in core if they matter, meaning that someone else seeing that the code doesn't satisfy the core lints should feel cautious about the code quality.
I don't think this lint qualifies.

@devoncarew
Copy link
Member

@kallentu - thanks for the proposal!

@chunhtai, it looks like @lrhn and @natebosch have already had a chance to review. Can you review this lint (esp. from a flutter context)? Some context:

  • originally landed in 259bf2445d6
  • it did ship in 3.7 stable
  • there is an associated quick fix

Please don't hesitate to ask questions (impact of the lint, ...).

@chunhtai
Copy link

From the flutter point of view, I don't think this lint will have much impact on flutter/flutter or flutter apps. flutter repro's style guide discourage the use of underscore.

I am more leaning toward @lrhn 's opinion as well

@devoncarew devoncarew moved this from Triage backlog to Under consideration in Lints Triage Feb 25, 2025
@devoncarew
Copy link
Member

After some discussion, we do want to add this to our lint sets (it'll help move code using older patterns to using wildcard variables).

There was some discussion of core or recommended. We settled on recommended, based in part on the scoring consideration (lints in core affect a package's pub score). In practice it won't matter much - most users use flutter_lints, which includes both core and recommended.

@devoncarew
Copy link
Member

Closed with #863.

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

No branches or pull requests

5 participants