-
Notifications
You must be signed in to change notification settings - Fork 77
feat: add support for configuration error severities in analysis_options.yaml
#326
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
base: main
Are you sure you want to change the base?
feat: add support for configuration error severities in analysis_options.yaml
#326
Conversation
@Artur-Wisniewski is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
To view this pull requests documentation preview, visit the following URL: docs.page/invertase/dart_custom_lint~326 Documentation is deployed and generated using docs.page. |
Thanks for this! Those tests are a bit too tied to the implementation details for my liking. Could you write e2e tests instead? Like running |
…nt in AnalysisErrorSeverity enum
Hello @rrousselGit 👋🏻 Please don't hesitate to let me know if there is anything else you need here. |
@@ -83,4 +83,72 @@ void main() { | |||
}, | |||
); | |||
}); | |||
|
|||
test('Respects configSeverities when converting errors', () { |
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.
Isn't this covered by the previous tests? I'd rather not have tests that are so tied to the implementation details.
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.
You're right. I will remove this test because it isn't needed here, and as you said, it is too much tied to the implementation details (now I see it).
@rrousselGit Hello, I don't know if you've seen my recent changes. Could you please check this PR in your free time? I'm keen for this feature to help me implement better programming practices in my project. |
Hey @rrousselGit could you check this PR please? |
} | ||
} else { | ||
serverErrors.add(convertAnalysisError(error, lineInfo: lineInfo)); | ||
List<analyzer.AnalysisError> errors, { |
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.
This feels like an odd place to have this logic.
But I can't immediately think of a better location due to AnalysisError being immutable.
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'd rather not change this file, as it is forked from analyzer. I want to keep it close to the source, for easy upgrade.
Could we maybe add a separate util such that:
dart_custom_lint/packages/custom_lint_builder/lib/src/client.dart
Lines 994 to 995 in f44a7cb
CustomAnalyzerConverter().convertAnalysisErrors( | |
allAnalysisErrors, |
Does something like:
...convertAnalysisError(...).map((error) => error.copyWith(error: <apply error config>))
Then the file stays untouched, and the location of the logic is in a more sensible place.
rule_name_1: error | ||
rule_name_2: warning | ||
rule_name_3: info | ||
rule_name_4: none |
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.
Afaik analyzer uses ignore
not none
:
errors:
unused_parameter: ignore
We should match
final value = entry.value; | ||
if (entry.key case final String key?) { | ||
final severity = ErrorSeverity.values.firstWhereOrNull( | ||
(e) => e.displayName == value, |
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.
Let's not rely on "displayName" .
There is no guarantee about its stability. This could indirectly break custom_lint
That has already happened to me before, so I won't fall for it twice :P
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.
Instead we can do a simple switch:
ErrorSeverity parseSeverity(String str) {
return switch (str) {
'error' => ErrorSeverity.error,
'ignore' => ErrorSeverity.none,
...,
_ => throw UnsupportedError('Unsupported severity $str'),
};
}
@@ -9,3 +9,7 @@ linter: | |||
public_member_api_docs: false | |||
avoid_print: false | |||
unreachable_from_main: false | |||
|
|||
custom_lint: |
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 think that is useful to have in the default example
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.
LGTM with some tiny changes :)
Closes issue: ISSUE-323
Now it is possible to override error severities from analysis_options.yaml using the errors parameter inside custom_lint
You should be able to do this:
All of the some_lint_with_info_severity lint's error messages will be shown as errors by the analyzer