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

Add unnecessary_underscores to core/recommended set. #856

Open
kallentu opened this issue Feb 3, 2025 · 4 comments
Open

Add unnecessary_underscores to core/recommended set. #856

kallentu opened this issue Feb 3, 2025 · 4 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Triage backlog
Development

No branches or pull requests

3 participants