-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Changes from 22 commits
0a65ef8
ccda0df
09a89f2
04253cc
e078f24
d61f271
a487e22
85219e8
021dd71
4908982
d05c928
2d89dd8
7e53502
5b8c9a8
f4eed3d
638ce34
c19c8f3
f8837ba
352580c
a6a2a78
5f94b32
bc0ce9f
38e5327
d725d68
e344ca7
814caf1
32faa24
9dc4b9d
1ccc307
3858e8d
dc86fa7
7a17ad8
55d6235
db09b06
220baab
4432af2
befac5a
006ead1
8719555
e1d0372
6940eee
37ccc08
f85c2fe
3d9dc63
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 != repoRoot.parent.path && | ||
| !Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| repoRoot = repoRoot.parent; | ||
| } | ||
| if (!Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| 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,241 @@ | ||
| // 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"'; | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
|
|
||
| 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 { | ||
|
camsim99 marked this conversation as resolved.
|
||
| // Find the repo root where the plugin tool is located. | ||
| Directory repoRoot = Directory.current; | ||
| while (repoRoot.path != repoRoot.parent.path && | ||
| !Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| repoRoot = repoRoot.parent; | ||
| } | ||
| if (!Directory(p.join(repoRoot.path, '.git')).existsSync()) { | ||
| print('❌ Could not find .git directory.'); | ||
| return false; | ||
| } | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
|
|
||
| // 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) { | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| print('❌ Failed to get staged files'); | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| 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. | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| 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( | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| (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.
Outdated
|
||
| final bool hasJava = stagedFiles.any((f) => f.endsWith('.java')); | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| 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(stdout.supportsAnsiEscapes ? '\x1B[2K\r' : '\n'); | ||
| } | ||
| 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
Contributor
Author
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. Refactored quite a bit and don't see how we would use this but if it's applicable here, let me know! |
||
| '--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.
Outdated
|
||
| 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( | ||
| stdout.supportsAnsiEscapes | ||
|
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. is this complexity needed? What value do we get from having ansiExcapes here. |
||
| ? '\x1B[2K\r✅ Formatting looks good.\n' | ||
| : '✅ Formatting looks good.\n', | ||
| ); | ||
| } | ||
|
|
||
| // 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.
Outdated
|
||
| 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.
Outdated
|
||
|
|
||
| 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. | ||
|
camsim99 marked this conversation as resolved.
Outdated
|
||
| 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.