-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[camera_android_camerax][tool] Integrate dart_code_linter for cyclomatic complexity checks #11999
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
Changes from 14 commits
14be833
f206376
0299ea1
30757fa
fc676e2
e82d111
5e798fe
8b336a2
2f96c6b
84abbf0
eb76af7
d93d538
dd58a0d
214f051
71559a9
24d9e8a
ea9770a
e9bdaeb
342eab8
64a8a90
db7e807
f745d82
d14f6e1
002182d
c1247d4
b934bf8
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,16 @@ | ||
| include: ../../../analysis_options.yaml | ||
|
|
||
| analyzer: | ||
| exclude: | ||
| - build/** | ||
| - android/** | ||
| - ios/** | ||
| - web/** | ||
| - windows/** | ||
| - macos/** | ||
| - linux/** | ||
|
|
||
| dart_code_linter: | ||
| metrics: | ||
| cyclomatic-complexity: 15 | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -313,6 +313,53 @@ class AnalyzeCommand extends PackageLoopingCommand { | |
| if (exitCode != 0) { | ||
| return PackageResult.fail(); | ||
| } | ||
|
|
||
| final customCheckRunners = <_CustomLinter>[ | ||
|
Contributor
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. Don't think we need this since we're only adding one linter at the moment
Contributor
Author
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 am expecting we will expand this list. I would like to keep the functionality for now. |
||
| _CustomLinter( | ||
| dependencyName: 'dart_code_linter', | ||
| run: _runDartCodeLinterForPackage, | ||
| ), | ||
| ]; | ||
|
|
||
| final Pubspec pubspec = package.parsePubspec(); | ||
| for (final runner in customCheckRunners) { | ||
| final bool hasDependency = pubspec.devDependencies.containsKey(runner.dependencyName); | ||
| if (hasDependency) { | ||
| final PackageResult result = await runner.run(package); | ||
| if (result.state == RunState.failed) { | ||
|
Collaborator
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. The strongly preferred pattern in the repo tooling is for the helper functions to return That way, we get all the results in every run, rather than only up to the first failing thing.
Contributor
Author
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. Yes sorry, I authored this pr at the same time as the other one you gave this feedback on and I didnt go back to check if the pattern was violated anywhere else. I also filed flutter/flutter#188867 which tracks changing the gradle behavior to follow this pattern. I will personally try to remember this pattern and I have taken steps on my local machine to try to ensure this pattern does not pop up again. |
||
| return result; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return PackageResult.success(); | ||
| } | ||
|
|
||
| /// Runs the `dart_code_linter` metrics analyzer on the package. | ||
| /// | ||
| /// Assumes `dart_code_linter` is present in `dev_dependencies`. | ||
| Future<PackageResult> _runDartCodeLinterForPackage(RepositoryPackage package) async { | ||
| print('Running dart_code_linter:metrics analysis...'); | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| final bool isFlutter = package.requiresFlutter(); | ||
| final String sdkCommand = isFlutter ? flutterCommand : _dartBinaryPath; | ||
| final int linterExitCode = await processRunner.runAndStream( | ||
| sdkCommand, | ||
| <String>[ | ||
| if (isFlutter) 'pub', | ||
| 'run', | ||
| 'dart_code_linter:metrics', | ||
| 'analyze', | ||
| 'lib', | ||
| '--set-exit-on-violation-level=warning', | ||
| ], | ||
|
reidbaker marked this conversation as resolved.
Outdated
|
||
| workingDir: package.directory, | ||
| ); | ||
| if (linterExitCode != 0) { | ||
| return PackageResult.fail(<String>[ | ||
| 'Metrics violations found. See the package\'s local "analysis_options.yaml" for configured thresholds.' | ||
|
reidbaker marked this conversation as resolved.
Outdated
|
||
| ]); | ||
| } | ||
|
|
||
| return PackageResult.success(); | ||
| } | ||
|
|
||
|
|
@@ -437,3 +484,20 @@ class AnalyzeCommand extends PackageLoopingCommand { | |
| return errors.isEmpty ? PackageResult.success() : PackageResult.fail(errors); | ||
| } | ||
| } | ||
|
|
||
| /// Represents a custom linter check that is executed during package analysis. | ||
| class _CustomLinter { | ||
| const _CustomLinter({ | ||
| required this.dependencyName, | ||
| required this.run, | ||
| }); | ||
|
|
||
| /// The name of the package dependency that triggers this custom check. | ||
| /// | ||
| /// The check is only executed if this dependency is listed in the package's | ||
| /// `dev_dependencies`. | ||
| final String dependencyName; | ||
|
|
||
| /// The runner function that executes the custom check. | ||
| final Future<PackageResult> Function(RepositoryPackage) run; | ||
| } | ||
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've audited this as safe to run in our CI and locally?
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.
SHA1 (dart_code_linter-4.1.5.tar.gz) = 86fa1bd240a88c98a4c45f0beb43b4e7adb32735
For auditability this this is the artifact I inspected.
Looking at the code I didn't see anything suspicious.
Some things I found but I do not think are blocking:
It uses https://pub.dev/packages/pub_updater to see if there is a newer version. It works without internet access.
There is a python script to make cursor skills. It is for contributors but I found that interesting.
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 concrete examples of suspicious things I looked for. Code that tried to spawn other process, code that tried to install other things, network calls, code that read env variables that looked like secrets.