Skip to content

Commit a2ad7e3

Browse files
[tool] Extract most conformance checks to separate validator classes (#11610)
Extracts the logic for many of the lightweight, repo-practice commands into separate validator classes, leaving just a minimal amount of logic in the command, in preparation for combining all of these into a single command. Doing so will make running conformance checks locally much easier since there will be fewer commands to run, and will make documenting both the tool itself, and the commands we want run against PRs (e.g., for agent instructions), easier as well. This is a mostly no-op code move, so that tests can change as little as possible in this PR. Then the next PR will then combine the commands, which will require a lot of test changes, but almost all of the non-test code will be able to stay unchanged because it's in the separate validator classes, which the combined command can call into as they are. All the new files were created by moving code from the corresponding command file, with very minimal changes (e.g., sometimes context that was a non-private getter from the base command class is a private field in the validator class, so underscores were added.) This is reflected in the fact that there are almost no test changes.
1 parent 8aa6c08 commit a2ad7e3

20 files changed

Lines changed: 3054 additions & 2703 deletions

script/tool/lib/src/common/file_utils.dart

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// found in the LICENSE file.
44

55
import 'package:file/file.dart';
6+
import 'package:path/path.dart' as p;
67

78
/// Returns a [File] created by appending all but the last item in [components]
89
/// to [base] as subdirectories, then appending the last as a file.
@@ -31,3 +32,20 @@ Directory childDirectoryWithSubcomponents(
3132
}
3233
return dir;
3334
}
35+
36+
/// Returns the relative path from [from] to [entity] using [platformContext]
37+
/// as the path context, but always formatting the result as a POSIX path
38+
/// (using forward slashes).
39+
///
40+
/// This is useful for generating paths that will be used in configuration
41+
/// files or command lines that expect POSIX paths, even when running on a
42+
/// platform that uses a different path separator, or for display.
43+
String relativePosixPath(
44+
FileSystemEntity entity, {
45+
required Directory from,
46+
required p.Context platformContext,
47+
}) => p.posix.joinAll(
48+
platformContext.split(
49+
platformContext.relative(entity.absolute.path, from: from.path),
50+
),
51+
);

script/tool/lib/src/common/package_looping_command.dart

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,10 @@
55
import 'dart:async';
66

77
import 'package:file/file.dart';
8-
import 'package:path/path.dart' as p;
98
import 'package:pub_semver/pub_semver.dart';
109

1110
import 'core.dart';
11+
import 'file_utils.dart';
1212
import 'git_version_finder.dart';
1313
import 'output_utils.dart';
1414
import 'package_command.dart';
@@ -238,8 +238,7 @@ abstract class PackageLoopingCommand extends PackageCommand {
238238
String getRelativePosixPath(
239239
FileSystemEntity entity, {
240240
required Directory from,
241-
}) =>
242-
p.posix.joinAll(path.split(path.relative(entity.path, from: from.path)));
241+
}) => relativePosixPath(entity, from: from, platformContext: path);
243242

244243
/// The suggested indentation for printed output.
245244
String get indentation => hasLongOutput ? '' : ' ';

script/tool/lib/src/dependabot_check_command.dart

Lines changed: 13 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -3,29 +3,20 @@
33
// found in the LICENSE file.
44

55
import 'package:file/file.dart';
6-
import 'package:yaml/yaml.dart';
76

8-
import 'common/output_utils.dart';
97
import 'common/package_looping_command.dart';
108
import 'common/repository_package.dart';
9+
import 'validators/dependabot_validator.dart';
1110

1211
/// A command to verify Dependabot configuration coverage of packages.
1312
class DependabotCheckCommand extends PackageLoopingCommand {
1413
/// Creates Dependabot check command instance.
15-
DependabotCheckCommand(super.packagesDir, {super.gitDir}) {
16-
argParser.addOption(
17-
_configPathFlag,
18-
help: 'Path to the Dependabot configuration file',
19-
defaultsTo: '.github/dependabot.yml',
20-
);
21-
}
22-
23-
static const String _configPathFlag = 'config';
14+
DependabotCheckCommand(super.packagesDir, {super.gitDir});
2415

2516
late Directory _repoRoot;
2617

27-
// The set of directories covered by "gradle" entries in the config.
28-
Set<String> _gradleDirs = const <String>{};
18+
// The set of directories covered by the repo's Dependabot configuration.
19+
late DependabotCoverage _coverage;
2920

3021
@override
3122
final String name = 'dependabot-check';
@@ -48,99 +39,26 @@ class DependabotCheckCommand extends PackageLoopingCommand {
4839
Future<void> initializeRun() async {
4940
_repoRoot = packagesDir.fileSystem.directory((await gitDir).path);
5041

51-
final config =
52-
loadYaml(
53-
_repoRoot
54-
.childFile(getStringArg(_configPathFlag))
55-
.readAsStringSync(),
56-
)
57-
as YamlMap;
58-
final dynamic entries = config['updates'];
59-
if (entries is! YamlList) {
60-
return;
61-
}
62-
63-
const typeKey = 'package-ecosystem';
64-
const dirKey = 'directory';
65-
const dirsKey = 'directories';
66-
final Iterable<YamlMap> gradleEntries = entries.cast<YamlMap>().where(
67-
(YamlMap entry) => entry[typeKey] == 'gradle',
68-
);
69-
final Iterable<String?> directoryEntries = gradleEntries.map(
70-
(YamlMap entry) => entry[dirKey] as String?,
71-
);
72-
final Iterable<String?> directoriesEntries = gradleEntries
73-
.map((YamlMap entry) => entry[dirsKey] as YamlList?)
74-
.expand((YamlList? list) => list?.nodes ?? <String>[])
75-
.cast<YamlScalar>()
76-
.map((YamlScalar entry) => entry.value as String);
77-
_gradleDirs = directoryEntries
78-
.followedBy(directoriesEntries)
79-
.whereType<String>()
80-
.toSet();
42+
_coverage = DependabotValidator.loadConfig(repoRoot: _repoRoot);
8143
}
8244

8345
@override
8446
Future<PackageResult> runForPackage(RepositoryPackage package) async {
85-
var skipped = true;
47+
final validator = DependabotValidator(
48+
coverage: _coverage,
49+
path: path,
50+
repoRoot: _repoRoot,
51+
indentation: indentation,
52+
);
53+
8654
final errors = <String>[];
8755

88-
final _GradleCoverageResult gradleResult =
89-
_validateDependabotGradleCoverage(package);
90-
skipped = skipped && gradleResult.runState == RunState.skipped;
91-
if (gradleResult.runState == RunState.failed) {
92-
printError('${indentation}Missing Gradle coverage.');
93-
print(
94-
'${indentation}Add a "gradle" entry to '
95-
'${getStringArg(_configPathFlag)} for ${gradleResult.missingPath}',
96-
);
97-
errors.add('Missing Gradle coverage');
98-
}
56+
errors.addAll(validator.validateDependabotCoverage(package));
9957

10058
// TODO(stuartmorgan): Add other ecosystem checks here as more are enabled.
10159

102-
if (skipped) {
103-
return PackageResult.skip('No supported package ecosystems');
104-
}
10560
return errors.isEmpty
10661
? PackageResult.success()
10762
: PackageResult.fail(errors);
10863
}
109-
110-
/// Returns the state for the Dependabot coverage of the Gradle ecosystem for
111-
/// [package]:
112-
/// - succeeded if it includes gradle and is covered.
113-
/// - failed if it includes gradle and is not covered.
114-
/// - skipped if it doesn't include gradle.
115-
_GradleCoverageResult _validateDependabotGradleCoverage(
116-
RepositoryPackage package,
117-
) {
118-
final Directory androidDir = package.platformDirectory(
119-
FlutterPlatform.android,
120-
);
121-
final Directory appDir = androidDir.childDirectory('app');
122-
if (appDir.existsSync()) {
123-
// It's an app, so only check for the app directory to be covered.
124-
final dependabotPath =
125-
'/${getRelativePosixPath(appDir, from: _repoRoot)}';
126-
return _gradleDirs.contains(dependabotPath)
127-
? _GradleCoverageResult(RunState.succeeded)
128-
: _GradleCoverageResult(RunState.failed, missingPath: dependabotPath);
129-
} else if (androidDir.existsSync()) {
130-
// It's a library, so only check for the android directory to be covered.
131-
final dependabotPath =
132-
'/${getRelativePosixPath(androidDir, from: _repoRoot)}';
133-
return _gradleDirs.contains(dependabotPath)
134-
? _GradleCoverageResult(RunState.succeeded)
135-
: _GradleCoverageResult(RunState.failed, missingPath: dependabotPath);
136-
}
137-
return _GradleCoverageResult(RunState.skipped);
138-
}
139-
}
140-
141-
class _GradleCoverageResult {
142-
_GradleCoverageResult(this.runState, {this.missingPath});
143-
144-
final RunState runState;
145-
final String? missingPath;
14664
}

0 commit comments

Comments
 (0)