Repository navigation
refactor(packages): reduce cognitive complexity of check licenses - #1757
Conversation
9c2875b to
7068cd8
Compare
|
@vgvbot /review-pr |
|
Code review submitted — PR approved. |
![unicoderbot[bot]](https://avatars.githubusercontent.com/in/3144111?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-executed refactor that reduces cognitive complexity in the check licenses command and workspace dependency resolver while preserving behavior. The decomposition into focused helpers (_validateLicenseOptions, _dependenciesToCheck, _collectLicenses, _retrieveLicenses, _bannedDependenciesFor, _composeLicenseListing) and the _WorkspaceDependencyCollector class is clean, and the private _LicenseRetrievalFailure exception elegantly consolidates the error-handling paths.
I verified behavior preservation on the subtle points:
_argResults.rest.firstOrNull ?? '.'matches the earlier length-guarded ternary because the> 1check throws before this line.- The exhaustive switch on
PubspecDependencyTypetooptionNameis equivalent to the old four-clause||chain (and will fail to compile if the enum ever gains a case, which is a reasonable tradeoff for exhaustiveness). - First-seen ordering in
_composeReportis preserved becauselicenseCount.entriesiterates insertion order, matching the previouslicenseTypes.toSet()LinkedHashSet behavior. totalLicenseCount = licenseTypes.lengthequals the previous sum-of-counts since each occurrence contributes one to both.- Default
bannedLicenseTypestoconst <String>{}safely replaces the old!= null && containsguard. _WorkspaceDependencyCollector.classify()preserves the directMain > directDev > directOverridden precedence (first iteration withputIfAbsenton an empty map is equivalent to the old unconditional assignment).- Progress message format and the
\nprefix on ignored-failure logs are both maintained.
No correctness issues found. The three complexity reductions in the PR description are real and the touched test surface should be unchanged.
|
Heads up — I'm auto-updating this PR by merging the latest |
1 similar comment
|
Heads up — I'm auto-updating this PR by merging the latest |
|
Heads up — I'm auto-updating this PR by merging the latest |
1 similar comment
|
Heads up — I'm auto-updating this PR by merging the latest |
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Declare _LicenseRetrievalFailure with a primary constructor. - Move the --dependency-type option name onto PubspecDependencyType. - Extract the workspace collector into its own part file, declare its logger through a primary constructor, and inject it into resolveWorkspaceDependencies so it can be tested in isolation.
Every dependency is assigned a license set, with retrieval failures recorded as unknown, so the nullable value type only forced dead null checks in the banned-license and report helpers.
PubspecDependencyType.optionName and dependencyTypeAllowedValues listed the same option names, so the CLI parser and very_good.yaml validator could drift from the type filter. The allowed values are now built from the enum, leaving optionName as the single source of truth.
|
Heads up — I'm auto-updating this PR by merging the latest |
2b10028 to
30d67fc
Compare
2 participants
Status
READY
Description
PackagesCheckLicensesCommand.runreads as early-return steps: validate options, load the lock file, select dependencies, collect licenses, report. Fetching one dependency's licenses lives in_retrieveLicenses, which throws a private_LicenseRetrievalFailurecarrying the message and exit code, caught in one place byrunand in one place by_collectLicenseswhen--ignore-retrieval-failuresis set.PubspecDependencyTypeto its option name._composeReporthands the per-package listing to_composeLicenseListing. Counts and their first-seen order are unchanged.resolveWorkspaceDependenciesmoves its nestedvisitclosure into a private_WorkspaceDependencyCollectorwith one method each for visiting a package, expanding a workspace entry, parsing a member and ranking dependency types. The walk is still depth-first and the warnings fire under the same conditions.Found by the full cognitive complexity scan in #1755, which tests VeryGoodOpenSource/very_good_workflows#520. Every function in the touched files now scores 15 or under. Behavior, log output, exit codes and public APIs are unchanged, and no tests were needed because the existing ones already reach every new branch.
PackagesCheckLicensesCommand.run_composeReportresolveWorkspaceDependenciesPart of a three-PR cleanup. Once all three merge, the full scan on #1755 passes:
Type of Change
🤖 Generated with Claude Code