Skip to content

[tool] Extract most conformance checks to separate validator classes#11610

Merged
auto-submit[bot] merged 8 commits intoflutter:mainfrom
stuartmorgan-g:tool-conformance-commands-pre-combination-refactor
May 4, 2026
Merged

[tool] Extract most conformance checks to separate validator classes#11610
auto-submit[bot] merged 8 commits intoflutter:mainfrom
stuartmorgan-g:tool-conformance-commands-pre-combination-refactor

Conversation

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

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.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label Apr 29, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the repository's validation logic by extracting validation rules into dedicated validator classes for Dependabot, Gradle, pubspec, README, repository information, and version/changelog checks. It also introduces a utility function for generating POSIX-compliant relative paths. The changes improve code modularity and maintainability by separating validation logic from command execution.

return succeeded;
}

/// Validates that [gradleLines] reads and uses a artifiact hub repository
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the documentation comment: "a artifiact" should be "an artifact".

Suggested change
/// Validates that [gradleLines] reads and uses a artifiact hub repository
/// Validates that [gradleLines] reads and uses an artifact hub repository
References
  1. Code should follow the relevant style guides for each language, which includes correct spelling and grammar in documentation. (link)

}
''';

/// Validates that [gradleLines] reads and uses a artifiact hub repository
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the documentation comment: "a artifiact" should be "an artifact".

Suggested change
/// Validates that [gradleLines] reads and uses a artifiact hub repository
/// Validates that [gradleLines] reads and uses an artifact hub repository
References
  1. Code should follow the relevant style guides for each language, which includes correct spelling and grammar in documentation. (link)

'''
$buildGradleName "namespace" must match the "package" attribute in AndroidManifest.xml, if one is present.
$buildGradleName namespace: "$namespace"
AndroidMastifest.xml package: "${packageMatch.group(1)}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the error message: "AndroidMastifest.xml" should be "AndroidManifest.xml".

Suggested change
AndroidMastifest.xml package: "${packageMatch.group(1)}"
AndroidManifest.xml package: "${packageMatch.group(1)}"
References
  1. Code should follow the relevant style guides for each language, which includes correct spelling in user-facing messages. (link)

if (implements == null) {
return 'Missing "implements: $expectedImplements" in "plugin" section.';
} else if (implements != expectedImplements) {
return 'Expecetd "implements: $expectedImplements"; '
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

There is a typo in the error message: "Expecetd" should be "Expected".

Suggested change
return 'Expecetd "implements: $expectedImplements"; '
return 'Expected "implements: $expectedImplements"; '
References
  1. Code should follow the relevant style guides for each language, which includes correct spelling in user-facing messages. (link)


String _issueTagForPackage(String packageName) {
// TODO(stuartmorgan): Move this to a config file. See
// https://github.com/flutter/flutter/issues/185364
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The issue link in the TODO appears to have a typo. Issue 185364 does not exist in the flutter/flutter repository, whereas issue 155364 specifically discusses moving issue labels to a config file, which matches the context of this TODO.

Suggested change
// https://github.com/flutter/flutter/issues/185364
// https://github.com/flutter/flutter/issues/155364

}

test('skips with no supported ecosystems', () async {
test('passes with no supported ecosystems', () async {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Skipping won't make sense in the combined command (no package skips all of the checks that will be in the combined command), so I didn't restructure the common validator pattern to allow it just for this command, only to remove it again in the next PR, and instead changed this behavior now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All the changes in this file are just because the constants moved.

p.split(p.relative(f.absolute.path, from: root.path)),
),
(File f) =>
relativePosixPath(f, from: root, platformContext: p.posix),
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Since I was extracting a helper for use in the validators (to replace the one in the looping command), I changed this to use the new helper that has the same logic.

'Allowing breaking change to plugin_platform_interface due '
'to --ignore-platform-interface-breaks',
'Ignoring breaking change to plugin_platform_interface due '
'to command configuration',
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to pass in the flag name to the validator just to keep this message the same. I don't think having the message be less descriptive is going to matter for a flag that's only run in CI, and in a message that's just for auditing if something weird ever happens. We'll still be able to trace the message back to this line.

Copy link
Copy Markdown
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions Bot removed the CICD Run CI/CD label May 4, 2026
@stuartmorgan-g stuartmorgan-g added autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD labels May 4, 2026
@auto-submit
Copy link
Copy Markdown
Contributor

auto-submit Bot commented May 4, 2026

autosubmit label was removed for flutter/packages/11610, because - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2026
@stuartmorgan-g stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label May 4, 2026
@auto-submit auto-submit Bot merged commit a2ad7e3 into flutter:main May 4, 2026
83 checks passed
pull Bot pushed a commit to mikeyhodl/flutter that referenced this pull request May 4, 2026
…r#186002)

flutter/packages@daf30f8...a2ad7e3

2026-05-04 stuartmorgan@google.com [tool] Extract most conformance
checks to separate validator classes (flutter/packages#11610)
2026-05-01 fluttergithubbot@gmail.com Sync release-go_router-17.2.3 to
main (flutter/packages#11633)
2026-05-01 engine-flutter-autoroll@skia.org Roll Flutter from
81bc3d6 to 707dbc0 (85 revisions) (flutter/packages#11630)
2026-05-01 stuartmorgan@google.com [google_maps_flutter] Fix styles on
web (flutter/packages#11629)
2026-05-01 engine-flutter-autoroll@skia.org Roll Flutter (stable) from
02085fe to 00b0c91 (3 revisions) (flutter/packages#11627)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants