-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add pre-commit hook to verify static analysis passes & format is correct #11941
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
base: main
Are you sure you want to change the base?
Changes from 20 commits
0a65ef8
ccda0df
09a89f2
04253cc
e078f24
d61f271
a487e22
85219e8
021dd71
4908982
d05c928
2d89dd8
7e53502
5b8c9a8
f4eed3d
638ce34
c19c8f3
f8837ba
352580c
a6a2a78
5f94b32
bc0ce9f
38e5327
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,32 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // ignore_for_file: avoid_print | ||
|
|
||
| import 'dart:io'; | ||
| import 'package:path/path.dart' as p; | ||
|
|
||
| void main() async { | ||
| Directory repoRoot = Directory.current; | ||
| while (repoRoot.path != '/' && !Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| repoRoot = repoRoot.parent; | ||
| } | ||
|
|
||
| if (repoRoot.path == '/') { | ||
| print('❌ Could not find .git directory.'); | ||
| exit(1); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
|
|
||
| final ProcessResult result = await Process.run('git', [ | ||
| 'config', | ||
| 'core.hooksPath', | ||
| 'script/githooks', | ||
| ], workingDirectory: repoRoot.path); | ||
| if (result.exitCode == 0) { | ||
| print('✅ Git hooks installed successfully!'); | ||
| } else { | ||
| print('❌ Failed to install Git hooks: ${result.stderr}'); | ||
| exit(1); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:io' as io; | ||
|
|
||
| import 'package:githooks/githooks.dart'; | ||
|
|
||
| Future<void> main(List<String> args) async { | ||
| io.exitCode = await run(args); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'package:args/command_runner.dart'; | ||
|
|
||
| import 'src/pre_commit_command.dart'; | ||
|
|
||
| /// Runs the githooks command line utility. | ||
| Future<int> run(List<String> args) async { | ||
| final runner = CommandRunner<bool>('githooks', 'Git hooks for flutter/packages') | ||
| ..addCommand(PreCommitCommand()); | ||
|
|
||
| final bool success = await runner.run(args) ?? false; | ||
| return success ? 0 : 1; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| // Copyright 2013 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // ignore_for_file: avoid_print | ||
|
|
||
| import 'dart:io'; | ||
| import 'package:args/command_runner.dart'; | ||
| import 'package:path/path.dart' as p; | ||
|
|
||
| /// The command that implements the pre-commit githook. | ||
| class PreCommitCommand extends Command<bool> { | ||
| /// Creates a [PreCommitCommand]. | ||
| PreCommitCommand({ | ||
| Future<ProcessResult> Function( | ||
| String executable, | ||
| List<String> arguments, { | ||
| String? workingDirectory, | ||
| })? | ||
| processRunner, | ||
| }) : processRunner = processRunner ?? Process.run; | ||
|
|
||
| /// The process runner injected for testing. | ||
| final Future<ProcessResult> Function( | ||
| String executable, | ||
| List<String> arguments, { | ||
| String? workingDirectory, | ||
| }) | ||
| processRunner; | ||
|
|
||
| @override | ||
| final String name = 'pre-commit'; | ||
|
|
||
| @override | ||
| final String description = 'Checks to run before a "git commit"'; | ||
|
|
||
| String? _findPackagePath(String filePath, String repoRoot) { | ||
| Directory currentDir = File(p.join(repoRoot, filePath)).parent; | ||
| while (p.isWithin(repoRoot, currentDir.path) || p.equals(repoRoot, currentDir.path)) { | ||
| final String dirName = p.basename(currentDir.path); | ||
| if (dirName != 'example' && File(p.join(currentDir.path, 'pubspec.yaml')).existsSync()) { | ||
| return currentDir.path; | ||
| } | ||
| if (p.equals(repoRoot, currentDir.path)) { | ||
| break; | ||
| } | ||
| currentDir = currentDir.parent; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @override | ||
| Future<bool> run() async { | ||
|
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. Add dart doc |
||
| // Find the repo root where the plugin tool is located. | ||
| Directory repoRoot = Directory.current; | ||
| while (repoRoot.path != '/' && !Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| repoRoot = repoRoot.parent; | ||
| } | ||
|
|
||
| if (repoRoot.path == '/') { | ||
| print('❌ Could not find .git directory.'); | ||
| return false; | ||
| } | ||
|
camsim99 marked this conversation as resolved.
camsim99 marked this conversation as resolved.
|
||
|
|
||
| // Get all staged files that are added, copied, or modified. | ||
| final ProcessResult diffResult = await processRunner('git', [ | ||
| 'diff', | ||
| '--cached', | ||
| '--name-only', | ||
| '--diff-filter=ACM', | ||
| ], workingDirectory: repoRoot.path); | ||
|
|
||
| if (diffResult.exitCode != 0) { | ||
|
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.
|
||
| print('❌ Failed to get staged files'); | ||
|
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. Do you think we should include the command that failed and the error code? |
||
| return false; | ||
| } | ||
|
|
||
| final List<String> stagedFiles = (diffResult.stdout as String) | ||
| .split('\n') | ||
| .map((e) => e.trim()) | ||
| .where((e) => e.isNotEmpty) | ||
| .toList(); | ||
|
|
||
| if (stagedFiles.isEmpty) { | ||
| // No files changed. | ||
| return true; | ||
| } | ||
|
|
||
| final Set<String> targetPackageDirs = {}; | ||
| for (final file in stagedFiles) { | ||
| final String? packageDir = _findPackagePath(file, repoRoot.path); | ||
| if (packageDir != null) { | ||
| targetPackageDirs.add(packageDir); | ||
| } | ||
| } | ||
|
|
||
| if (targetPackageDirs.isEmpty) { | ||
| // None of the changed files are part of a package we care about. | ||
|
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. I am a bit confused as to what this does. It looks like it is building a set of packages from staged files. But if that is the case then where does the filtering happen. |
||
| return true; | ||
| } | ||
|
|
||
| final Set<String> targetPackages = targetPackageDirs.map((dir) => p.basename(dir)).toSet(); | ||
|
|
||
| final String toolScript = p.join( | ||
| repoRoot.path, | ||
| 'script', | ||
| 'tool', | ||
| 'bin', | ||
| 'flutter_plugin_tools.dart', | ||
| ); | ||
| final packageArgs = '--packages=${targetPackages.join(',')}'; | ||
|
|
||
| // Determine which toolchains are needed based on file extensions to avoid uneccessary slowdown. | ||
| final bool hasClang = stagedFiles.any( | ||
|
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. What code determines if this list needs to change? |
||
| (f) => | ||
| f.endsWith('.c') || | ||
| f.endsWith('.cc') || | ||
| f.endsWith('.cpp') || | ||
| f.endsWith('.h') || | ||
| f.endsWith('.m') || | ||
| f.endsWith('.mm'), | ||
| ); | ||
|
camsim99 marked this conversation as resolved.
|
||
| final bool hasJava = stagedFiles.any((f) => f.endsWith('.java')); | ||
|
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. Recommend structuring this a bit more formally. What information is needed per language? How do you execute per language. Lets make the blast radius clear and the file manipulations happen outside of any language logic. |
||
| final bool hasKotlin = stagedFiles.any((f) => f.endsWith('.kt')); | ||
| final bool hasSwift = stagedFiles.any((f) => f.endsWith('.swift')); | ||
|
|
||
| final List<String> dartFiles = stagedFiles.where((f) => f.endsWith('.dart')).toList(); | ||
|
|
||
| print( | ||
| '🏃 Running pre-commit checks on ${targetPackages.length} packages: ${targetPackages.join(', ')}', | ||
| ); | ||
| var hasError = false; | ||
|
|
||
| // Check formatting. | ||
| stdout.write('Checking formatting...'); | ||
|
|
||
| // Format staged Dart files. | ||
| if (dartFiles.isNotEmpty) { | ||
| final ProcessResult dartFormatResult = await processRunner('dart', [ | ||
| 'format', | ||
| '--set-exit-if-changed', | ||
| ...dartFiles, | ||
| ], workingDirectory: repoRoot.path); | ||
|
|
||
| if (dartFormatResult.exitCode != 0) { | ||
| if (!hasError) { | ||
| stdout.write('\x1B[2K\r'); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
| if (dartFormatResult.stdout.toString().isNotEmpty) { | ||
| print(dartFormatResult.stdout); | ||
| } | ||
| if (dartFormatResult.stderr.toString().isNotEmpty) { | ||
| print(dartFormatResult.stderr); | ||
| } | ||
| print('❌ Formatting issues found in Dart files. Please run "dart format" to fix them.'); | ||
| hasError = true; | ||
| } | ||
| } | ||
|
|
||
| // Format staged native files. | ||
| final bool needsNativeFormat = hasClang || hasJava || hasKotlin || hasSwift; | ||
| if (needsNativeFormat) { | ||
| final nativeFormatFlags = [ | ||
|
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. Lets use the args package. https://pub.dev/packages/args |
||
| '--no-dart', | ||
| if (!hasClang) '--no-clang-format', | ||
| if (!hasJava) '--no-java', | ||
| if (!hasKotlin) '--no-kotlin', | ||
| if (!hasSwift) '--no-swift', | ||
| ]; | ||
|
|
||
| final ProcessResult nativeFormatResult = await processRunner('dart', [ | ||
| 'run', | ||
| toolScript, | ||
| 'format', | ||
| packageArgs, | ||
| '--fail-on-change', | ||
| ...nativeFormatFlags, | ||
| ], workingDirectory: repoRoot.path); | ||
|
|
||
| if (nativeFormatResult.exitCode != 0) { | ||
| if (!hasError) { | ||
| stdout.write('\x1B[2K\r'); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
| if (nativeFormatResult.stdout.toString().isNotEmpty) { | ||
| print(nativeFormatResult.stdout); | ||
| } | ||
| if (nativeFormatResult.stderr.toString().isNotEmpty) { | ||
| print(nativeFormatResult.stderr); | ||
| } | ||
| print( | ||
| '❌ Formatting issues found in native files. Please run "dart run script/tool/bin/flutter_plugin_tools.dart format $packageArgs" to fix them.', | ||
|
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. I am guessing ai wrote this script because it tends to use these unicode chars for success and failure. Lets remove those unicode chars. |
||
| ); | ||
| hasError = true; | ||
| } | ||
| } | ||
|
|
||
| if (!hasError) { | ||
| stdout.write('\x1B[2K\r✅ Formatting looks good.\n'); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
|
|
||
| // Run static analysis on staged files. | ||
| var analyzeHasError = false; | ||
| stdout.write('Running static analysis...'); | ||
|
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. Structurally my gut is organizing code by job type instead of by when in the hooks process the code runs is more likely to be a durable standard. |
||
| if (dartFiles.isNotEmpty) { | ||
| final ProcessResult analyzeResult = await processRunner('dart', [ | ||
| 'analyze', | ||
| '--fatal-infos', | ||
| ...dartFiles, | ||
| ], workingDirectory: repoRoot.path); | ||
|
|
||
| if (analyzeResult.exitCode != 0) { | ||
| if (!analyzeHasError) { | ||
| stdout.write('\x1B[2K\r'); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
| if (analyzeResult.stdout.toString().isNotEmpty) { | ||
| print(analyzeResult.stdout); | ||
| } | ||
| if (analyzeResult.stderr.toString().isNotEmpty) { | ||
| print(analyzeResult.stderr); | ||
| } | ||
| print('❌ Static analysis errors found.'); | ||
| analyzeHasError = true; | ||
| hasError = true; | ||
| } | ||
| } | ||
|
|
||
| if (!analyzeHasError) { | ||
| stdout.write('\x1B[2K\r✅ Static analysis looks good.\n'); | ||
| } | ||
|
camsim99 marked this conversation as resolved.
|
||
|
|
||
| if (hasError) { | ||
| print('❌ Please fix the errors listed above.'); | ||
| } | ||
|
|
||
| return !hasError; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| #!/usr/bin/env bash | ||
| set -e | ||
|
|
||
| HOOKS_DIR="$(dirname "$0")" | ||
| exec dart "$HOOKS_DIR/bin/main.dart" pre-commit "$@" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| name: githooks | ||
| description: Git hooks for the flutter/packages repository. | ||
| publish_to: none | ||
|
|
||
| environment: | ||
| sdk: ^3.10.0-0 | ||
|
|
||
| dependencies: | ||
| args: any | ||
| path: any | ||
|
|
||
| dev_dependencies: | ||
| test: any | ||
|
camsim99 marked this conversation as resolved.
|
||
Uh oh!
There was an error while loading. Please reload this page.