[L] Change in dart/sdk[main]: [DAS] Adds some warnings for the `analysis_options` file
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Felipe MorschelAlright. Since we decided on reverting that base change, I'll update this to include the base change too.
I'll also update the title and description for more context.
I've rebased this on the older change and added the tests for `true`/`false` that were missing last time.
Can I also ask someone to reopen the issue since that got reverted, please? Thanks!
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Felipe Morschel
- Konstantin Shcheglov
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Rebased this change. Can I get some reviews please, @sche...@google.com and @sraw...@google.com? Thanks a lot!
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 5 comments![]( "Open in Gerrit")
I didn't spend a lot of time looking at either the implementation or the tests (which is why I'm not giving it a +1), so it wasn't clear to me exactly what the design is. (If I had to write documentation for these warnings, and I might, I wouldn't know what to write.) Is there a design discussion somewhere?
The kinds of things I'm thinking about are
- Does it catch conflicts within a single file?
- Does it catch conflicts between the current file and an included file? (I know the answer to that one is 'yes'.
- Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
- Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?
Without knowing which rule the user would prefer to use, we cannot removeI'm not asking for the fixes to be implemented, but we could potentially provide both fixes (when there are only two incompatible rules) and just let the user choose one.
I'd change this to 'needsFix' and describe that option.
reporter.atSourceSpan(Should this be using `incompatibleIncludedLint`? That helper method creates a context message, but this doesn't. It would be nice to create the context message everywhere.
problemMessage: "The rule '{0}' is incompatible with the rule '{1}' enabled at '{2}'."I'm fine with this being an improvement in a follow-on CL, but this phrase of the message would be better implemented as a context message (and dropped from the problem message).
Also, consider dropping the second "the rule". I think it's clear from context that we're talking about rules.
problemMessage: "This file enables '{0}' that is incompatible with '{1}' enabled in another include."Consider "The rule '{0}' is incompatible with '{1}', which is enabled in an included file."
Related details
- Felipe Morschel
- Konstantin Shcheglov
- Samuel Rawlins
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
Without knowing which rule the user would prefer to use, we cannot removeI'm not asking for the fixes to be implemented, but we could potentially provide both fixes (when there are only two incompatible rules) and just let the user choose one.
I'd change this to 'needsFix' and describe that option.
Will do that, just note that https://github.com/dart-lang/sdk/issues/55985 is still open and for the fixes to be implemented, that issue should probably be resolved.
Related details
- Brian Wilkerson
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
I didn't spend a lot of time looking at either the implementation or the tests (which is why I'm not giving it a +1), so it wasn't clear to me exactly what the design is. (If I had to write documentation for these warnings, and I might, I wouldn't know what to write.) Is there a design discussion somewhere?
The kinds of things I'm thinking about are
- Does it catch conflicts within a single file?
- Does it catch conflicts between the current file and an included file? (I know the answer to that one is 'yes'.
- Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
- Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?
Is there a design discussion somewhere?
Maybe some specific comments at the [original CL](https://dart-review.googlesource.com/c/sdk/+/419982) but nothing beyond, that I'm aware of. We could use the linked issue for this. It is open specifically for this CL, nothing more.
Does it catch conflicts within a single file?
No, but we may use the previous warning for this case, nice catch!
Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
It should but no specific lint for this case, we might need a better message.
Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?
Same as the above answer. It would also be a problem with the currently existing lint, it will only warn for a single incompatibility, not sure if it triggers multiple times for that case, we may need to check.
Related details
- Brian Wilkerson
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
Felipe MorschelI didn't spend a lot of time looking at either the implementation or the tests (which is why I'm not giving it a +1), so it wasn't clear to me exactly what the design is. (If I had to write documentation for these warnings, and I might, I wouldn't know what to write.) Is there a design discussion somewhere?
The kinds of things I'm thinking about are
- Does it catch conflicts within a single file?
- Does it catch conflicts between the current file and an included file? (I know the answer to that one is 'yes'.
- Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
- Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?
Is there a design discussion somewhere?
Maybe some specific comments at the [original CL](https://dart-review.googlesource.com/c/sdk/+/419982) but nothing beyond, that I'm aware of. We could use the linked issue for this. It is open specifically for this CL, nothing more.
Does it catch conflicts within a single file?
No, but we may use the previous warning for this case, nice catch!
Does it catch multiple conflicts, such as having two included files that enable a rule that conflicts with the current file?
It should but no specific lint for this case, we might need a better message.
Does it catch multiple conflicts, such as enabling a rule that conflicts with multiple other rules multiple of which are also enabled?
Same as the above answer. It would also be a problem with the currently existing lint, it will only warn for a single incompatibility, not sure if it triggers multiple times for that case, we may need to check.
- Felipe Morschel
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 4 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
4 comments
I'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
Should this be using `incompatibleIncludedLint`? That helper method creates a context message, but this doesn't. It would be nice to create the context message everywhere.
Done
problemMessage: "The rule '{0}' is incompatible with the rule '{1}' enabled at '{2}'."I'm fine with this being an improvement in a follow-on CL, but this phrase of the message would be better implemented as a context message (and dropped from the problem message).
Also, consider dropping the second "the rule". I think it's clear from context that we're talking about rules.
I'll answer here for all of the other comments and your design request on the issue:
I think the diagnostics now reflect what you asked. I'm using the factory everywhere and passing the related data as context messages, The messages are now updated to reflect your requests, please review them to make sure this is what you were asking, thanks.
problemMessage: "This file enables '{0}' that is incompatible with '{1}' enabled in another include."Consider "The rule '{0}' is incompatible with '{1}', which is enabled in an included file."
- Brian Wilkerson
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
I'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
I think I've managed to make this work. Any tips on tests that verify the (conunt of, at least) context messages?
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
Felipe MorschelI'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
I think I've managed to make this work. Any tips on tests that verify the (conunt of, at least) context messages?
Maybe you'll know @brianwi...@google.com. Any idea on how to count the amount of context messages?
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
Felipe MorschelI'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
Felipe MorschelI think I've managed to make this work. Any tips on tests that verify the (conunt of, at least) context messages?
Maybe you'll know @brianwi...@google.com. Any idea on how to count the amount of context messages?
Friendly ping @brianwi...@google.com.
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 2 comments![]( "Open in Gerrit")
Felipe MorschelI'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
Felipe MorschelI think I've managed to make this work. Any tips on tests that verify the (conunt of, at least) context messages?
Felipe MorschelMaybe you'll know @brianwi...@google.com. Any idea on how to count the amount of context messages?
Friendly ping @brianwi...@google.com.
Thanks for the ping. This had slipped off my radar.
Any idea on how to count the amount of context messages?
Use `assertErrorsInCode`, which takes a list of `ExpectedError` objects, created by the `error` utility method. Those objects can specify the number of context messages, though possibly only by including the full message text. (We could add a way to specify only the number of messages if that would be better.)
INCOMPATIBLE_LINT_INCLUDED:I can't tell the difference between this and `INCOMPATIBLE_LINT_FILES`. Is there a reason for two seemingly identical codes? If not, consider merging the two (and I like the message for `INCOMPATIBLE_LINT_INCLUDED` better than the one for `INCOMPATIBLE_LINT_FILES`).
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
INCOMPATIBLE_LINT_INCLUDED:I can't tell the difference between this and `INCOMPATIBLE_LINT_FILES`. Is there a reason for two seemingly identical codes? If not, consider merging the two (and I like the message for `INCOMPATIBLE_LINT_INCLUDED` better than the one for `INCOMPATIBLE_LINT_FILES`).
I can't tell the difference
That is not great then.
Is there a reason for two seemingly identical codes?
The idea is that the "included" version would trigger for rules found within the current file that are incompatible with the included files.
The "files" version would trigger for conflicts between included files, but not necessarily related to the current one.
But if you dislike the idea of both being different, I'm open to merge them. I just wanted us to have a way of differentiating the cases of this lint better within our code. Or if you have any idea on how to better explain the different cases within the messages, I'm open to that as well.
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
INCOMPATIBLE_LINT_INCLUDED:Felipe MorschelI can't tell the difference between this and `INCOMPATIBLE_LINT_FILES`. Is there a reason for two seemingly identical codes? If not, consider merging the two (and I like the message for `INCOMPATIBLE_LINT_INCLUDED` better than the one for `INCOMPATIBLE_LINT_FILES`).
I can't tell the difference
That is not great then.
Is there a reason for two seemingly identical codes?
The idea is that the "included" version would trigger for rules found within the current file that are incompatible with the included files.
The "files" version would trigger for conflicts between included files, but not necessarily related to the current one.
But if you dislike the idea of both being different, I'm open to merge them. I just wanted us to have a way of differentiating the cases of this lint better within our code. Or if you have any idea on how to better explain the different cases within the messages, I'm open to that as well.
Thanks, now I think I understand. Having that distinction would be valuable, both for the user's comprehension and so that our fixes (if we have any) would know what problem is being fixed (I'm assuming the fixes for the two cases might differ some).
The "files" version could probably be worded better. My first take would be to use a message similar to "The lint 'L1', enabled in 'include1', isn't compatible with the lint 'L2', enabled in 'include2' and 'include3'.". I think that would contain the data a user would want. Either "enabled in" clause could be a list for full flexibility.
Alternatively, and maybe better, would be "The lint 'L1' isn't compatible with the lint 'L2'." and context messages of the form "'L1' is enabled in 'include1'." for each of the places where either lint is enabled. That would enable navigation to all of the location.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
Felipe MorschelI'm still working on making two tests pass (`YamlNode.span` seems to be without `sourceUrl`). When I fix that I'll resolve this comment and ping you all again.
Felipe MorschelI think I've managed to make this work. Any tips on tests that verify the (conunt of, at least) context messages?
Felipe MorschelMaybe you'll know @brianwi...@google.com. Any idea on how to count the amount of context messages?
Brian WilkersonFriendly ping @brianwi...@google.com.
Thanks for the ping. This had slipped off my radar.
Any idea on how to count the amount of context messages?
Use `assertErrorsInCode`, which takes a list of `ExpectedError` objects, created by the `error` utility method. Those objects can specify the number of context messages, though possibly only by including the full message text. (We could add a way to specify only the number of messages if that would be better.)
I think specifying them was easy enough. Thanks!
INCOMPATIBLE_LINT_INCLUDED:Felipe MorschelI can't tell the difference between this and `INCOMPATIBLE_LINT_FILES`. Is there a reason for two seemingly identical codes? If not, consider merging the two (and I like the message for `INCOMPATIBLE_LINT_INCLUDED` better than the one for `INCOMPATIBLE_LINT_FILES`).
Brian WilkersonI can't tell the difference
That is not great then.
Is there a reason for two seemingly identical codes?
The idea is that the "included" version would trigger for rules found within the current file that are incompatible with the included files.
The "files" version would trigger for conflicts between included files, but not necessarily related to the current one.
But if you dislike the idea of both being different, I'm open to merge them. I just wanted us to have a way of differentiating the cases of this lint better within our code. Or if you have any idea on how to better explain the different cases within the messages, I'm open to that as well.
Thanks, now I think I understand. Having that distinction would be valuable, both for the user's comprehension and so that our fixes (if we have any) would know what problem is being fixed (I'm assuming the fixes for the two cases might differ some).
The "files" version could probably be worded better. My first take would be to use a message similar to "The lint 'L1', enabled in 'include1', isn't compatible with the lint 'L2', enabled in 'include2' and 'include3'.". I think that would contain the data a user would want. Either "enabled in" clause could be a list for full flexibility.
Alternatively, and maybe better, would be "The lint 'L1' isn't compatible with the lint 'L2'." and context messages of the form "'L1' is enabled in 'include1'." for each of the places where either lint is enabled. That would enable navigation to all of the location.
I've done some more work here and I think this is better (preferred your last suggestion). I'll leave this unresolved so you can confirm this is aligned with what you/the team would like.
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 2 comments![]( "Open in Gerrit")
INCOMPATIBLE_LINT_INCLUDED:Felipe MorschelI can't tell the difference between this and `INCOMPATIBLE_LINT_FILES`. Is there a reason for two seemingly identical codes? If not, consider merging the two (and I like the message for `INCOMPATIBLE_LINT_INCLUDED` better than the one for `INCOMPATIBLE_LINT_FILES`).
Brian WilkersonI can't tell the difference
That is not great then.
Is there a reason for two seemingly identical codes?
The idea is that the "included" version would trigger for rules found within the current file that are incompatible with the included files.
The "files" version would trigger for conflicts between included files, but not necessarily related to the current one.
But if you dislike the idea of both being different, I'm open to merge them. I just wanted us to have a way of differentiating the cases of this lint better within our code. Or if you have any idea on how to better explain the different cases within the messages, I'm open to that as well.
Felipe MorschelThanks, now I think I understand. Having that distinction would be valuable, both for the user's comprehension and so that our fixes (if we have any) would know what problem is being fixed (I'm assuming the fixes for the two cases might differ some).
The "files" version could probably be worded better. My first take would be to use a message similar to "The lint 'L1', enabled in 'include1', isn't compatible with the lint 'L2', enabled in 'include2' and 'include3'.". I think that would contain the data a user would want. Either "enabled in" clause could be a list for full flexibility.
Alternatively, and maybe better, would be "The lint 'L1' isn't compatible with the lint 'L2'." and context messages of the form "'L1' is enabled in 'include1'." for each of the places where either lint is enabled. That would enable navigation to all of the location.
I've done some more work here and I think this is better (preferred your last suggestion). I'll leave this unresolved so you can confirm this is aligned with what you/the team would like.
I think the approach is reasonable, thanks! Others can speak up if they disagree.
(file) => [We aren't using this parameter in the body, consider replacing it with `_`. The same is true on 156.
I'm actually not seeing where the parameter is ever used. If I missed it, please let me know. If it isn't used anywhere, then consider changing this argument back to being just a list, rather than a function that returns a list.
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
(file) => [We aren't using this parameter in the body, consider replacing it with `_`. The same is true on 156.
I'm actually not seeing where the parameter is ever used. If I missed it, please let me know. If it isn't used anywhere, then consider changing this argument back to being just a list, rather than a function that returns a list.
I added it to test the base case and forgot about it, sorry. I've used it now but just in two tests, do you think we can handle the reference to the testFile somewhere else or is it OK for us to do this change?
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
(file) => [Felipe MorschelWe aren't using this parameter in the body, consider replacing it with `_`. The same is true on 156.
I'm actually not seeing where the parameter is ever used. If I missed it, please let me know. If it isn't used anywhere, then consider changing this argument back to being just a list, rather than a function that returns a list.
I added it to test the base case and forgot about it, sorry. I've used it now but just in two tests, do you think we can handle the reference to the testFile somewhere else or is it OK for us to do this change?
Given that the `file` will always be known to the test, I'd just hard code it into the expectations and not pass to a closure.
I'd do that by adding a field (maybe `analysisOptionsFile`?) to `AbstractAnalysisOptionsTest`, initializing it to the `File` the tests all use, then using it in `assertErrorsInCode` rather that creating the file there (to prevent them from getting out of sync).
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
(file) => [Felipe MorschelWe aren't using this parameter in the body, consider replacing it with `_`. The same is true on 156.
I'm actually not seeing where the parameter is ever used. If I missed it, please let me know. If it isn't used anywhere, then consider changing this argument back to being just a list, rather than a function that returns a list.
Brian WilkersonI added it to test the base case and forgot about it, sorry. I've used it now but just in two tests, do you think we can handle the reference to the testFile somewhere else or is it OK for us to do this change?
Given that the `file` will always be known to the test, I'd just hard code it into the expectations and not pass to a closure.
I'd do that by adding a field (maybe `analysisOptionsFile`?) to `AbstractAnalysisOptionsTest`, initializing it to the `File` the tests all use, then using it in `assertErrorsInCode` rather that creating the file there (to prevent them from getting out of sync).
I had to make it a `late` field because I need to `get` it's value before entering `assertErrorsInCode` (to build the expected error). But I think the changes are fine. I'll leave this as unresolved so you can confirm. Thanks!
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 9 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
9 comments
message: 'Incompatible lint rule.',It would be better if the message could include the name of the lint rule being 'pointed to'. Perhaps something like "The rule '${incompatible.value.toString()}' is enabled here.".
message: "'${incompatible.toString()}' declared in '$file'.",If we change the message above then we might want to change this one similarly. Maybe something like "The rule '${incompatible.value.toString()}' is enabled here in the file '$file'."
message: 'Incompatible lint rule.',This should match the context message used for `incompatibleLintFiles`. There's no value in having them be different, and it makes it easier for users to find help online if all of the discussions include the same text.
correctionMessage: "Try removing one of the incompatible rules.""one" --> "all but one"
If there are multiple rules that are mutually exclusive, removing one of them won't fix the problem, it will just reduce the number of conflicting rules.
problemMessage: "The rule '{0}' isn't compatible with {1}.""isn't compatible" --> "incompatible" (for consistency)
correctionMessage: "Try removing the incompatible files or locally disable one of the conflicting rules.""disable one"--> "disabling all but one"
I think we should invert these two options because I think that locally disabling a rule is going to be the more practical solution in more cases. I'm not even sure we want to suggest removing an include because there is likely to be other lints or settings that the user doesn't want to loose.
correctionMessage: "Try removing one of the incompatible file or locally disable one of the conflicting rules.""disable one"--> "disabling all but one"
(Same comment about the order.)
correctionMessage: "Try removing one of the incompatible file or locally disable one of the conflicting rules.""file" --> "files"
(file) => [Felipe MorschelWe aren't using this parameter in the body, consider replacing it with `_`. The same is true on 156.
I'm actually not seeing where the parameter is ever used. If I missed it, please let me know. If it isn't used anywhere, then consider changing this argument back to being just a list, rather than a function that returns a list.
Brian WilkersonI added it to test the base case and forgot about it, sorry. I've used it now but just in two tests, do you think we can handle the reference to the testFile somewhere else or is it OK for us to do this change?
Felipe MorschelGiven that the `file` will always be known to the test, I'd just hard code it into the expectations and not pass to a closure.
I'd do that by adding a field (maybe `analysisOptionsFile`?) to `AbstractAnalysisOptionsTest`, initializing it to the `File` the tests all use, then using it in `assertErrorsInCode` rather that creating the file there (to prevent them from getting out of sync).
I had to make it a `late` field because I need to `get` it's value before entering `assertErrorsInCode` (to build the expected error). But I think the changes are fine. I'll leave this as unresolved so you can confirm. Thanks!
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 9 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
9 comments
It would be better if the message could include the name of the lint rule being 'pointed to'. Perhaps something like "The rule '${incompatible.value.toString()}' is enabled here.".
Done
message: "'${incompatible.toString()}' declared in '$file'.",If we change the message above then we might want to change this one similarly. Maybe something like "The rule '${incompatible.value.toString()}' is enabled here in the file '$file'."
Done
This should match the context message used for `incompatibleLintFiles`. There's no value in having them be different, and it makes it easier for users to find help online if all of the discussions include the same text.
Great idea! Thanks!
correctionMessage: "Try removing one of the incompatible rules.""one" --> "all but one"
If there are multiple rules that are mutually exclusive, removing one of them won't fix the problem, it will just reduce the number of conflicting rules.
Done
problemMessage: "The rule '{0}' isn't compatible with {1}.""isn't compatible" --> "incompatible" (for consistency)
Done
correctionMessage: "Try removing the incompatible files or locally disable one of the conflicting rules.""disable one"--> "disabling all but one"
I think we should invert these two options because I think that locally disabling a rule is going to be the more practical solution in more cases. I'm not even sure we want to suggest removing an include because there is likely to be other lints or settings that the user doesn't want to loose.
Great idea!
correctionMessage: "Try removing one of the incompatible file or locally disable one of the conflicting rules."Felipe Morschel"file" --> "files"
Done
correctionMessage: "Try removing one of the incompatible file or locally disable one of the conflicting rules.""disable one"--> "disabling all but one"
(Same comment about the order.)
- Brian Wilkerson
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
if (expected.expectedContextMessages.isNotEmpty) {@sraw...@google.com, I saw your issue https://github.com/dart-lang/sdk/issues/61271 and I was wondering if you think about updating this code similarly too.
I'd be glad to do it with this CL since I'm here already but another is fine by me as well. But of course that if you want to tackle this yourself, feel free to do so.
Related details
- Brian Wilkerson
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
if (expected.expectedContextMessages.isNotEmpty) {@sraw...@google.com, I saw your issue https://github.com/dart-lang/sdk/issues/61271 and I was wondering if you think about updating this code similarly too.
I'd be glad to do it with this CL since I'm here already but another is fine by me as well. But of course that if you want to tackle this yourself, feel free to do so.
Ah good catch. No I don't think it needs to be updated here as well.
There is a lot of unfortunate duplication in our various base test classes. But for analyzer compile-time errors and warnings, we don't use the same `lint()` shorthand. Each expectation must spell out exactly which code is expected.
We have long term plans to de-duplicate the test code, likely into a shared location like the analyzer_testing package. But it's on the backburner.
Related details
- Brian Wilkerson
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Alright. After the last bot run I fixed the two problems I found and did some refactoring to make it easier to read. I think the next bots run should be fine.
Can someone please run it for me? Thanks a lot! If it all goes well, I'm ready for the last reviews here.
Related details
- Brian Wilkerson
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 3 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
3 comments
var includes = switch (includeValue) {I agree this is simpler.
void _processEnabledRule({I like splitting this into a new method.
Can you add a doc comment describing the parameters? In particular, `activeRules` needs some text about how it is modified, and how it is subsequently called with the updated value.
required List<AbstractAnalysisRule> disabledRules,I don't see how this parameter is used. Can it be removed?
Related details
- Brian Wilkerson
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
I like splitting this into a new method.
Can you add a doc comment describing the parameters? In particular, `activeRules` needs some text about how it is modified, and how it is subsequently called with the updated value.
Thanks for noting it. I actually made yet another small refactor to modify it where I was calling this method (after it) and made some changes to accept only a `rules` parameter that encompasses both imported rules and rules from this file. I think it is now easier to reason about. Let me know what you think!
Closing this so you can open new comments for the current file state.
I don't see how this parameter is used. Can it be removed?
Oh yes, thanks!
I actually thought we had a warning for this on private methods, but I guess I was wrong about when it would trigger. It does so when you never assign it any value but not when you don't use it...
I guess I'll raise this at https://github.com/dart-lang/sdk/issues/60587
Related details
- Brian Wilkerson
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 3 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
3 comments
This is looking great! A really great feature to report these!
if (incompatible.where((data) => data.file == null) case var listCan we use a different name here, instead of `list`. This is something like `localIncompatible`, right?
if (incompatible.where((data) => data.file != null) case var listSame here, `includedIncompatible`?
Related details
- Brian Wilkerson
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
This is looking great! A really great feature to report these!
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 2 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
2 comments
if (incompatible.where((data) => data.file == null) case var listCan we use a different name here, instead of `list`. This is something like `localIncompatible`, right?
Sure!
if (incompatible.where((data) => data.file != null) case var listFelipe MorschelSame here, `includedIncompatible`?
Done
Related details
- Brian Wilkerson
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Hey, @sche...@google.com, mind giving me your review here please? Thanks a lot!
Related details
- Brian Wilkerson
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Code-Review+1![]( "Open in Gerrit")
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
Hey, @sche...@google.com, mind giving me your review here please? Thanks a lot!
@sraw...@google.com or @brianwi...@google.com, can you give me one more review to land this again properly, please? Thanks a lot!
Related details
- Brian Wilkerson
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
Felipe MorschelHey, @sche...@google.com, mind giving me your review here please? Thanks a lot!
@sraw...@google.com or @brianwi...@google.com, can you give me one more review to land this again properly, please? Thanks a lot!
- Brian Wilkerson
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
Unfortunatelly, there have been recent changes in how the diagnostics are now generated and this CL was stale.
I've rebased it and fixed the bots. Can I get a new run please? Thanks!
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Code-Review+1![]( "Open in Gerrit")
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Felipe Morschel
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
[DAS] Adds some warnings for the `analysis_options` file
Adds `incompatible_lint` for included rules (and some variants of it) and `unsupported_value` error for linter rules values.
Original change: https://dart-review.googlesource.com/c/sdk/+/419982
Original change reverted at: https://dart-review.googlesource.com/c/sdk/+/429381
Fixes: https://github.com/dart-lang/sdk/issues/60125
- M pkg/analysis_server/lib/src/context_manager.dart
- M pkg/analysis_server/lib/src/handler/legacy/edit_get_fixes.dart
- M pkg/analysis_server/lib/src/lsp/handlers/code_actions/analysis_options.dart
- M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
- M pkg/analysis_server/test/services/linter/linter_test.dart
- M pkg/analysis_server/test/src/services/correction/fix/analysis_options/test_support.dart
- M pkg/analyzer/lib/src/analysis_options/analysis_options_provider.dart
- M pkg/analyzer/lib/src/analysis_options/error/option_codes.g.dart
- M pkg/analyzer/lib/src/analysis_options/options_file_validator.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic_code_values.g.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic_factory.dart
- M pkg/analyzer/lib/src/lint/options_rule_validator.dart
- M pkg/analyzer/messages.yaml
- M pkg/analyzer/test/generated/test_support.dart
- M pkg/analyzer/test/src/diagnostics/analysis_options/analysis_options_test_support.dart
- M pkg/analyzer/test/src/options/options_file_validator_test.dart
- M pkg/analyzer/test/src/options/options_rule_validator_test.dart
- M pkg/analyzer_cli/lib/src/driver.dart
Code-Review: +1 by Samuel Rawlins, +1 by Konstantin Shcheglov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |