-
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?
Changes from all commits
ae42d24
53f2a59
3c11288
1595a84
2fba63c
3f3a6a1
4f11ecc
135070a
374e367
c398b9d
04b7f96
774d137
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
import 'dart:convert'; | ||
import 'dart:io'; | ||
|
||
import 'package:analyzer/error/error.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
import 'cli_process_test.dart'; | ||
import 'create_project.dart'; | ||
import 'peer_project_meta.dart'; | ||
|
||
void main() { | ||
group('Errors severities override', () { | ||
Future<ProcessResult> runProcess(String workingDirectoryPath) async => | ||
Process.run( | ||
'dart', | ||
[customLintBinPath], | ||
workingDirectory: workingDirectoryPath, | ||
stdoutEncoding: utf8, | ||
stderrEncoding: utf8, | ||
); | ||
|
||
Directory createLintUsageWith({ | ||
required Uri pluginUri, | ||
required String analysisOptions, | ||
}) => | ||
createLintUsage( | ||
name: 'test_app', | ||
source: {'lib/main.dart': 'void fn() {}'}, | ||
plugins: {'test_lint': pluginUri}, | ||
analysisOptions: analysisOptions, | ||
); | ||
|
||
Directory createTestPlugin({ | ||
ErrorSeverity errorSeverity = ErrorSeverity.INFO, | ||
}) => | ||
createPlugin( | ||
name: 'test_lint', | ||
main: createPluginSource([ | ||
TestLintRule( | ||
code: 'test_lint', | ||
message: 'Test lint message', | ||
errorSeverity: errorSeverity, | ||
), | ||
]), | ||
); | ||
test('correctly applies error severity from analysis_options.yaml', | ||
() async { | ||
final plugin = createTestPlugin(errorSeverity: ErrorSeverity.ERROR); | ||
|
||
final app = createLintUsageWith( | ||
pluginUri: plugin.uri, | ||
analysisOptions: ''' | ||
custom_lint: | ||
errors: | ||
test_lint: error | ||
''', | ||
); | ||
|
||
final process = await runProcess(app.path); | ||
|
||
expect(trimDependencyOverridesWarning(process.stderr), isEmpty); | ||
expect(process.stdout, ''' | ||
Analyzing... | ||
|
||
lib/main.dart:1:6 • Test lint message • test_lint • ERROR | ||
|
||
1 issue found. | ||
'''); | ||
expect(process.exitCode, 1); | ||
}); | ||
|
||
test('correctly applies warning severity from analysis_options.yaml', | ||
() async { | ||
final plugin = createTestPlugin(); | ||
|
||
final app = createLintUsageWith( | ||
pluginUri: plugin.uri, | ||
analysisOptions: ''' | ||
custom_lint: | ||
errors: | ||
test_lint: warning | ||
''', | ||
); | ||
|
||
final process = await runProcess(app.path); | ||
|
||
expect(trimDependencyOverridesWarning(process.stderr), isEmpty); | ||
expect(process.stdout, ''' | ||
Analyzing... | ||
|
||
lib/main.dart:1:6 • Test lint message • test_lint • WARNING | ||
|
||
1 issue found. | ||
'''); | ||
expect(process.exitCode, 1); | ||
}); | ||
|
||
test('correctly applies info severity from analysis_options.yaml', | ||
() async { | ||
final plugin = createTestPlugin(); | ||
|
||
final app = createLintUsageWith( | ||
pluginUri: plugin.uri, | ||
analysisOptions: ''' | ||
custom_lint: | ||
errors: | ||
test_lint: info | ||
''', | ||
); | ||
|
||
final process = await runProcess(app.path); | ||
|
||
expect(trimDependencyOverridesWarning(process.stderr), isEmpty); | ||
expect(process.stdout, ''' | ||
Analyzing... | ||
|
||
lib/main.dart:1:6 • Test lint message • test_lint • INFO | ||
|
||
1 issue found. | ||
'''); | ||
expect(process.exitCode, 1); | ||
}); | ||
|
||
test('correctly applies none severity from analysis_options.yaml', | ||
() async { | ||
final plugin = createTestPlugin(); | ||
|
||
final app = createLintUsageWith( | ||
pluginUri: plugin.uri, | ||
analysisOptions: ''' | ||
custom_lint: | ||
errors: | ||
test_lint: none | ||
''', | ||
); | ||
|
||
final process = await runProcess(app.path); | ||
|
||
expect(trimDependencyOverridesWarning(process.stderr), isEmpty); | ||
expect(process.stdout, ''' | ||
Analyzing... | ||
|
||
No issues found! | ||
'''); | ||
expect(process.exitCode, 0); | ||
}); | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -72,23 +72,28 @@ class CustomAnalyzerConverter { | |||||
/// start column. If an analysis [options] is provided then the severities of | ||||||
/// the errors will be altered based on those options. | ||||||
List<plugin.AnalysisError> convertAnalysisErrors( | ||||||
List<analyzer.AnalysisError> errors, | ||||||
{analyzer.LineInfo? lineInfo, | ||||||
analyzer.AnalysisOptions? options}) { | ||||||
var serverErrors = <plugin.AnalysisError>[]; | ||||||
for (var error in errors) { | ||||||
var processor = analyzer.ErrorProcessor.getProcessor(options, error); | ||||||
if (processor != null) { | ||||||
var severity = processor.severity; | ||||||
// Errors with null severity are filtered out. | ||||||
if (severity != null) { | ||||||
// Specified severities override. | ||||||
serverErrors.add(convertAnalysisError(error, | ||||||
lineInfo: lineInfo, severity: severity)); | ||||||
} | ||||||
} else { | ||||||
serverErrors.add(convertAnalysisError(error, lineInfo: lineInfo)); | ||||||
List<analyzer.AnalysisError> errors, { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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
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. |
||||||
analyzer.LineInfo? lineInfo, | ||||||
analyzer.AnalysisOptions? options, | ||||||
Map<String, analyzer.ErrorSeverity>? configSeverities, | ||||||
}) { | ||||||
final serverErrors = <plugin.AnalysisError>[]; | ||||||
for (final error in errors) { | ||||||
final processor = analyzer.ErrorProcessor.getProcessor(options, error); | ||||||
final configSeverity = configSeverities?[error.errorCode.name]; | ||||||
// Config severities override processor severities | ||||||
final severity = configSeverity ?? processor?.severity; | ||||||
|
||||||
// Errors with none severity are filtered out. | ||||||
if (severity == analyzer.ErrorSeverity.NONE) { | ||||||
continue; | ||||||
} | ||||||
|
||||||
serverErrors.add(convertAnalysisError( | ||||||
error, | ||||||
lineInfo: lineInfo, | ||||||
severity: severity, | ||||||
)); | ||||||
} | ||||||
return serverErrors; | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import 'package:analyzer/error/error.dart'; | ||
import 'package:analyzer/file_system/file_system.dart'; | ||
import 'package:collection/collection.dart'; | ||
import 'package:meta/meta.dart'; | ||
|
@@ -17,6 +18,7 @@ class CustomLintConfigs { | |
required this.verbose, | ||
required this.debug, | ||
required this.rules, | ||
required this.errors, | ||
}); | ||
|
||
/// Decode a [CustomLintConfigs] from a file. | ||
|
@@ -108,11 +110,33 @@ class CustomLintConfigs { | |
} | ||
} | ||
|
||
final errors = <String, ErrorSeverity>{...includedOptions.errors}; | ||
|
||
final errorsYaml = customLint['errors'] as Object?; | ||
if (errorsYaml is Map) { | ||
for (final entry in errorsYaml.entries) { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Let's not rely on "displayName" . 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 commentThe 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'),
};
} |
||
); | ||
if (severity == null) { | ||
throw ArgumentError( | ||
'Provided error severity: $value specified for key: $key is not valid. ' | ||
'Valid error severities are: ${ErrorSeverity.values.map((e) => e.displayName).join(', ')}', | ||
); | ||
} | ||
errors[key] = severity; | ||
} | ||
} | ||
} | ||
|
||
return CustomLintConfigs( | ||
enableAllLintRules: enableAllLintRules, | ||
verbose: verbose, | ||
debug: debug, | ||
rules: UnmodifiableMapView(rules), | ||
errors: UnmodifiableMapView(errors), | ||
); | ||
} | ||
|
||
|
@@ -123,6 +147,7 @@ class CustomLintConfigs { | |
verbose: false, | ||
debug: false, | ||
rules: {}, | ||
errors: {}, | ||
); | ||
|
||
/// A field representing whether to enable/disable lint rules that are not | ||
|
@@ -147,20 +172,26 @@ class CustomLintConfigs { | |
/// Whether enable hot-reload and log the VM-service URI. | ||
final bool debug; | ||
|
||
/// A map of lint rules to their severity. This is used to override the severity | ||
/// of a lint rule for a specific lint. | ||
final Map<String, ErrorSeverity> errors; | ||
|
||
@override | ||
bool operator ==(Object other) => | ||
other is CustomLintConfigs && | ||
other.enableAllLintRules == enableAllLintRules && | ||
other.verbose == verbose && | ||
other.debug == debug && | ||
const MapEquality<String, LintOptions>().equals(other.rules, rules); | ||
const MapEquality<String, LintOptions>().equals(other.rules, rules) && | ||
const MapEquality<String, ErrorSeverity>().equals(other.errors, errors); | ||
|
||
@override | ||
int get hashCode => Object.hash( | ||
enableAllLintRules, | ||
verbose, | ||
debug, | ||
const MapEquality<String, LintOptions>().hash(rules), | ||
const MapEquality<String, ErrorSeverity>().hash(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.
I don't think that is useful to have in the default example