-
Notifications
You must be signed in to change notification settings - Fork 170
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
[Wildcard Variables] support for non_constant_identifier_names
#5048
Comments
SGTM. Treat That wouldn't applyt to named constructor names, which can be private, so
|
Fixes: dart-lang/linter#5048 Change-Id: I87cb13737f73d4207d3cf57c2682b420ab084a7a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/379502 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Phil Quitslund <pquitslund@google.com>
Fixed w/ dart-lang/sdk@cc63001 |
While making a flag flip CL, I'm coming across this lint a lot. This lint doesn't seem like the right wording + fix.
|
And then following up on the above, this is also blocking the flag flip roll out, because we're in a catch 22 with flipping the flag and flutter/engine linting on the flag flip pre-submits. Knowing that this lint message isn't exactly what we wanted, could we possibly make a new lint with an diagnostic message like "Don't use multiple underscores, use one underscore to get a wildcard variable" ? We revert this change, so maybe don't lint on So 1. revert this lint. Thoughts? |
Right yeah, this is what we agreed on at the time but we absolutely can do better. I see a few choices. The best seem to be:
The costs of adding a new lint make me inclined towards the second option. As for the fix, we currently have a Thoughts? Input welcome on error messaging too. (That's often the toughest part!) /fyi @bwilkerson |
Do you mean just making it less opinionated about |
Yes, sorry. I meant making it less opinionated about |
Current plan:
(3 and 4 can happen in parallel.) |
I don't know what our release process is suppose to look like, but it's my understanding that once we flip the flag (to make the feature enabled by default) there's no going back, and the feature will ship in the next dev and stable builds. I'd like to propose that if existing lints are currently broken, or if there are lints we want to have available when the feature ships, then we aren't ready to flip the flag. In addition, the analyzer and analysis server work doesn't appear to be complete, based on the tracking issues for them. It feels like we're being a bit hasty to be flipping the flag at this point. And yes, we should absolutely have a fix in place to convert multi-underscore names to a single underscore where doing so is allowed, and that fix needs to be usable by |
Great points Brian! We should definitely discuss. One quick thought.
I agree. In this case, I wouldn't say |
For the underscore case, we're currently producing a message that reads like:
Would something like this be too verbose?
Note that the lint currently does not try and establish if
(If unused.) And maybe just fall back to the existing message if it is:
|
Another wrinkle: there's potential overlap/interference w/ The more I think about it, maybe nudging folks to wildcards is better done there? See: #5040 |
The But yes, I think that being a local variable is a better signal than being a constant. I would think we'd want to flag non-constant local variables in the same way, but we wouldn't want to flag non-local constants. I agree that |
A few arguments for adding a new lint instead of using either of those two:
|
Yes, thanks, Kallen! #5077 proposes a new lint. Feedback welcome! |
As of dart-lang/sdk@b2bdedf, |
We're currently special-casing
__
,___
, etc. in a number of places that we should update with the availability of wildcards.Specifically we should now flag multiple underscores in:
Exception. We also special case constructor declarations but I think that should not change and we should continue to all
A.__()
,A.___()
, etc. (See discussion in #1854.)/cc @kallentu @lrhn @bwilkerson
The text was updated successfully, but these errors were encountered: