Skip to content
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 92 additions & 0 deletions lib/src/commands/packages/commands/check/commands/licenses.dart
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import 'package:package_config/package_config.dart' as package_config;
import 'package:pana/src/license_detection/license_detector.dart' as detector;
import 'package:path/path.dart' as path;
import 'package:very_good_cli/src/pub_license/spdx_license.gen.dart';
import 'package:very_good_cli/src/pubspec/pubspec.dart';
import 'package:very_good_cli/src/pubspec_lock/pubspec_lock.dart';

/// Overrides the [package_config.findPackageConfig] function for testing.
Expand Down Expand Up @@ -192,11 +193,48 @@ class PackagesCheckLicensesCommand extends Command<int> {
return ExitCode.noInput.code;
}

// Check if this is a workspace root and collect dependencies accordingly
final pubspecFile = File(path.join(targetPath, pubspecBasename));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: you've already computed targetPath and targetDirectory. path.join(targetDirectory.path, pubspecBasename) would avoid recomputing the join from targetPath and keeps the single source of truth (targetDirectory). Same comment applies to pubspecLockFile above.

final pubspec = tryParsePubspec(pubspecFile);

// Collect workspace dependencies if this is a workspace root
final workspaceDependencies = _collectWorkspaceDependencies(
pubspec: pubspec,
targetDirectory: targetDirectory,
dependencyTypes: dependencyTypes,
);

final filteredDependencies = pubspecLock.packages.where((dependency) {
if (!dependency.isPubHosted) return false;

if (skippedPackages.contains(dependency.name)) return false;

// If we have workspace dependencies, use them for filtering direct deps
if (workspaceDependencies != null) {
// For direct-main and direct-dev, check against workspace dependencies
if (dependencyTypes.contains('direct-main') ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This outer if is redundant given the way workspaceDependencies is built — the Set only contains names whose declared type is in dependencyTypes, so the inner contains already encodes the filter. Either drop this branch (and just check workspaceDependencies.contains(dependency.name)) or, if you want to actually enforce direct-main vs direct-dev separately, change _collectWorkspaceDependencies to return per-type Sets. As written it implies a distinction that isn't really being made.

dependencyTypes.contains('direct-dev')) {
if (workspaceDependencies.contains(dependency.name)) {
return true;
}
}

// For transitive and direct-overridden, still use pubspec.lock types
final dependencyType = dependency.type;
if (dependencyTypes.contains('transitive') &&
dependencyType == PubspecLockPackageDependencyType.transitive) {
return true;
}
if (dependencyTypes.contains('direct-overridden') &&
dependencyType ==
PubspecLockPackageDependencyType.directOverridden) {
return true;
}

return false;
}

// Non-workspace: use the original filtering logic
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtle behavioral divergence worth calling out in a comment or doc: in non-workspace mode, direct-main is decided by pubspec.lock's recorded type; in workspace mode, it's decided by which members declare it. For overlapping packages (a dep that's direct dev in pubspec.lock but appears in some member's dependencies:) the two paths can disagree. Probably fine, but users will notice.

final dependencyType = dependency.type;
return (dependencyTypes.contains('direct-main') &&
dependencyType == PubspecLockPackageDependencyType.directMain) ||
Expand Down Expand Up @@ -531,6 +569,60 @@ extension on List<Object> {
}
}

/// Collects dependencies from a workspace.
///
/// If [pubspec] is not a workspace root, returns `null`.
/// Otherwise, returns a set of dependency names collected from all workspace
/// members based on the requested [dependencyTypes].
Set<String>? _collectWorkspaceDependencies({
required Pubspec? pubspec,
required Directory targetDirectory,
required List<String> dependencyTypes,
}) {
if (pubspec == null || !pubspec.isWorkspaceRoot) return null;

final dependencies = <String>{};

// Collect dependencies from the root pubspec itself
if (dependencyTypes.contains('direct-main')) {
dependencies.addAll(pubspec.dependencies.keys);
}
if (dependencyTypes.contains('direct-dev')) {
dependencies.addAll(pubspec.devDependencies.keys);
}

// Collect dependencies from workspace members
final members = resolveWorkspaceMembers(pubspec, targetDirectory);
for (final memberDirectory in members) {
final memberPubspecFile = File(
path.join(memberDirectory.path, pubspecBasename),
);
final memberPubspec = tryParsePubspec(memberPubspecFile);
if (memberPubspec == null) continue;

if (dependencyTypes.contains('direct-main')) {
dependencies.addAll(memberPubspec.dependencies.keys);
}
if (dependencyTypes.contains('direct-dev')) {
dependencies.addAll(memberPubspec.devDependencies.keys);
}

// Handle nested workspaces recursively
if (memberPubspec.isWorkspaceRoot) {
final nestedDeps = _collectWorkspaceDependencies(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no cycle protection here. If a member's pubspec.yaml lists the parent (or itself) under workspace: — e.g. via a misconfigured glob like .. — this recursion will not terminate. Consider passing a Set<String> of canonicalized directory paths and short-circuiting if memberDirectory.absolute.path has already been visited.

pubspec: memberPubspec,
targetDirectory: memberDirectory,
dependencyTypes: dependencyTypes,
);
if (nestedDeps != null) {
dependencies.addAll(nestedDeps);
}
}
}

return dependencies;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper is purely about workspace traversal and doesn't depend on anything in this command — it would fit better in lib/src/pubspec/pubspec.dart (or a sibling) alongside resolveWorkspaceMembers, keeping the licenses command focused. That also makes it easier to unit-test directly rather than only through the integration tests.


/// Format type for listing all licenses via --reporter option.
enum ReporterOutputFormat {
/// List all licenses separated by a dash.
Expand Down
84 changes: 84 additions & 0 deletions lib/src/pubspec/pubspec.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/// Workspace-aware helpers around [package:pubspec_parse].
///
/// Parsing is delegated to [Pubspec.parse]; this library only adds the
/// filesystem and glob expansion helpers needed by
/// `packages check licenses` to walk workspace members.
library;

import 'dart:io';

import 'package:glob/glob.dart';
import 'package:glob/list_local_fs.dart';
import 'package:path/path.dart' as path;
import 'package:pubspec_parse/pubspec_parse.dart';

export 'package:pubspec_parse/pubspec_parse.dart' show Pubspec;

/// The basename of the pubspec file.
const pubspecBasename = 'pubspec.yaml';

const _workspaceResolution = 'workspace';

/// Workspace-related conveniences on top of [Pubspec].
extension PubspecWorkspace on Pubspec {
/// Whether this pubspec is a workspace root.
bool get isWorkspaceRoot => workspace != null;

/// Whether this pubspec is a workspace member.
bool get isWorkspaceMember => resolution == _workspaceResolution;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isWorkspaceMember isn't referenced anywhere in lib/; only the tests call it. Either delete it (YAGNI), or wire it into resolveWorkspaceMembers/_collectWorkspaceDependencies to warn/skip when a resolved member doesn't declare resolution: workspace — that would catch real misconfigurations rather than just being inert.

}

/// Attempts to read and parse a [Pubspec] from [file].
///
/// Returns `null` when the file does not exist or cannot be parsed.
Pubspec? tryParsePubspec(File file) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additional to the bot's comments, I'd say all the pubspec related methods can be in the existing PubspecWorkspace extension since we're just extending pubspec_parse

if (!file.existsSync()) return null;
try {
return Pubspec.parse(
file.readAsStringSync(),
sourceUrl: file.uri,
lenient: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lenient: true causes Pubspec.parse to accept many structurally invalid pubspecs without throwing. That means tryParsePubspec only returns null for true YAML-level errors — an otherwise well-formed YAML doc with bad dependencies: shape will parse to a Pubspec with empty maps and we'll silently skip its deps. The malformed-pubspec integration test exercises the YAML-error path; consider either dropping lenient: true (so structural errors are surfaced as well), or logging a warning when a workspace member fails to parse so users aren't left wondering why a member's deps are missing.

);
} on Exception {
return null;
}
}

/// Resolves the workspace members declared by [pubspec], expanding any glob
/// patterns relative to [root].
///
/// Returns an empty list when [pubspec] is not a workspace root.
List<Directory> resolveWorkspaceMembers(Pubspec pubspec, Directory root) {
final patterns = pubspec.workspace;
if (patterns == null) return const [];

final members = <Directory>[];
for (final pattern in patterns) {
if (_isGlobPattern(pattern)) {
final matches = Glob(pattern).listSync(root: root.path);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workspace patterns under pub use POSIX-style separators, but on Windows users may write packages\app (or it may end up that way after normalization). Glob is POSIX-only — consider normalizing patterns to POSIX before constructing the Glob, or document the constraint.

for (final match in matches) {
if (match is Directory) {
final pubspecFile = File(path.join(match.path, pubspecBasename));
if (pubspecFile.existsSync()) {
members.add(Directory(match.path));
}
} else if (match is File &&
path.basename(match.path) == pubspecBasename) {
members.add(Directory(match.parent.path));
}
}
} else {
final memberDir = Directory(path.join(root.path, pattern));
if (memberDir.existsSync()) members.add(memberDir);
}
}

return members;
}

bool _isGlobPattern(String pattern) {
return pattern.contains('*') ||
pattern.contains('?') ||
pattern.contains('[') ||
pattern.contains('{');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hand-rolling glob detection is fragile — e.g. negation patterns starting with ! won't be flagged as globs and will be passed to Directory(path.join(root.path, pattern)), while a literal directory name containing { or [ (uncommon but legal) will be misclassified the other way. Since pkg:glob is already a dependency, you can avoid the heuristic entirely by always constructing a Glob(pattern) and letting it handle literal vs pattern paths uniformly, or by checking via Glob(pattern).pattern characteristics.

}
Loading
Loading