This repository has been archived by the owner on Nov 20, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a lint for missing files in conditional imports #3080
Add a lint for missing files in conditional imports #3080
Changes from 1 commit
a5382e1
a399507
e850de7
e967a02
aabc623
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for naming, I might suggest that we use a name similar to the error code (
URI_DOES_NOT_EXIST
) such asconditional_uri_does_not_exist
. There's a companion error code in the analyzer (URI_HAS_NOT_BEEN_GENERATED
) and we might want to have two separate messages for this lint to distinguish between those cases, but that's less clear to me.(The only convention that I'm aware of for naming lints is that we're now actively discouraging using 'avoid', 'prefer' and other such designations from the style guide because those tend to change over time leaving us with misleading lint names. Other than that, lints should be named to either express the condition they're trying to ensure or the condition they're trying to prevent.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've changed it to
conditional_uri_does_not_exist
.I don't know much about that so have no opinion, but if you think it would also be useful here I'm happy to add it - it doesn't seem like a complicated change (just changing the code/error depending on whether the uri ends with these suffixes?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it a bit more, I'd propose that we leave it as is. If we get feedback from users that a different message would be helpful we can always update it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For performance reasons, the linter uses visitors as a dispatch mechanism rather than as a tree walking mechanism. Walking the tree is expensive, so we do it once and effectively run all of the lint rules simultaneously. As a result,
_Visitor
should extendSimpleAstVisitor
,addConfiguration
rather thanaddCompilationUnit
on line 43, andsuper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, apparently the file I duplicated as a starting point was the only one using a RecursiveAstVisitor (
missing_whitespace_between_adjacent_strings.dart
) 😄This makes sense and I've made those changes, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That rule should definitely be updated. (But obviously not an issue for this PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, wasn't sure if there was a reason. I'll file a PR, seems like a trivial change. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "Try" --> "try"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you noted, this will probably work most of the time, but it would be better if we could ask the resource provider. Unfortunately there doesn't appear to be any way to access it currently.
My personal opinion is that we need to update
LinterContext
to provide access to the resource provider and then pass that in when constructing the visitor. Unfortunately, that's a multi-step process, and I'm not sure we want to delay merging this rule while that happens.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to go with your decision here. If you think it's worth doing (and I know enough to do it), I'm happy to. Although I'm not sure if having to support
package:
URIs complicates using the resource provider. Thisexists()
call seemed to handle that (anddart:
URIs) for me which I thought might have complicated things :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with leaving it as-is. You might consider adding a comment for future reference, because the fact that it isn't checking for overlays might not occur to a future reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Would this lint be invalidated/updated correctly if the user saved a new file with the name that was used here? Would the server know if a file has
import 'file_a.dart'
but that file does not exist, that creating a file at that location may require this file to be re-analyzed/linted?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know whether it is the case, but it ought to be re-analyzed when a new file is added, and we run lints as part of analyzing a file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the preferred style for tests, thanks!