-
-
Notifications
You must be signed in to change notification settings - Fork 85
WIP: support language: dart
#1146
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: master
Are you sure you want to change the base?
WIP: support language: dart
#1146
Conversation
Implements full Dart language support following TDD approach: - Migrated tests from pre-commit's test suite - Implemented Dart language handler in src/languages/dart.rs - System Dart installation detection - pubspec.yaml dependency installation via `dart pub get` - Additional dependencies via `dart pub cache add` - Environment setup with PUB_CACHE - Added comprehensive test suite covering: - Health checks - Script execution with file arguments - pubspec.yaml handling - Additional dependencies (with and without versions) - Environment variable setup - Error handling - Updated documentation in docs/todo.md Language features: - Uses system-installed Dart (no version management) - Supports additional_dependencies - Supports environment setup - Compatible with pre-commit Dart hooks Closes j178#51
- Use which::which() crate instead of 'which' command (cross-platform) - Parse Dart version robustly by finding 'Dart SDK version:' line - Handle Flutter SDK which outputs extra lines before version - Combine stdout/stderr as dart --version may use either Fixes test failures on Windows where 'which' command doesn't exist.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1146 +/- ##
===========================================
- Coverage 89.62% 25.74% -63.88%
===========================================
Files 74 75 +1
Lines 13733 13937 +204
===========================================
- Hits 12308 3588 -8720
- Misses 1425 10349 +8924 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Enhanced query_dart_info() with debug logging for PATH and executable detection - Added diagnostic_dart_detection test to help troubleshoot Windows PATH issues - Improved error message when dart executable cannot be found - Logs PATH variable, executable location, and version output for debugging This will help diagnose why tests fail to find dart.bat on Windows even when it's available in the user's shell environment.
📦 Cargo Bloat ComparisonBinary size change: +0.00% (16.3 MiB → 16.3 MiB) Expand for cargo-bloat outputHead Branch ResultsBase Branch Results |
The diagnostic test revealed that `which::which("dart")` successfully finds
the dart executable, but `Cmd::new("dart", ...)` was failing to find it.
Root cause: We were finding the dart executable with which::which(), but then
passing just "dart" to Cmd::new(), which uses a different lookup mechanism
that doesn't handle .bat files properly on Windows.
Solution: Use the full executable path we already found with which::which()
in all Cmd::new() calls instead of relying on Cmd to find it again.
Changes:
- query_dart_info(): Use `Cmd::new(&executable, ...)` instead of `Cmd::new("dart", ...)`
- install_from_pubspec(): Find dart path and use it in Cmd
- install_dependency(): Find dart path and use it in Cmd
This ensures consistent executable resolution across all operations and
properly handles Windows .bat wrapper files.
The previous implementation used `dart pub cache add` which only downloads packages but doesn't make them importable. In Dart, packages need to be declared in pubspec.yaml and resolved with `dart pub get` to be usable. Changes: - Replaced install_dependency() with install_additional_dependencies() - Creates a minimal pubspec.yaml in env_path with all additional dependencies - Runs `dart pub get` to properly resolve and install packages - Handles version specifications (e.g., "package:1.8.0" or "package") This creates the .dart_tool/package_config.json that makes imports work. Fixes the additional_dependencies and additional_dependencies_with_version tests.
Added regex filter to normalize Dart SDK version output in tests. This handles both standalone Dart and Flutter SDK Dart installations, which can have different output formats. Filter: "Dart SDK version: .*" -> "Dart SDK version: [VERSION]" Fixes the health_check snapshot test that was showing full version instead of the filtered [VERSION] placeholder.
When additional_dependencies are installed, dart pub get creates a .dart_tool/package_config.json in env_path that tells Dart where to find packages. However, dart scripts run from work_dir, where there's no package_config.json, causing "package not found" errors. Solution: Before running dart commands, copy the package_config.json from env_dir/.dart_tool to work_dir/.dart_tool if it exists. This allows Dart to properly resolve package imports when running hooks with additional_dependencies. Fixes additional_dependencies and additional_dependencies_with_version tests.
The diagnostic test served its purpose in identifying the Windows PATH issue and is no longer needed now that all tests are passing.
Added Dart SDK installation to all CI test jobs (Linux, macOS, Windows) to ensure Dart language tests can run successfully in CI. Changes: - Added DART_VERSION: "3.10.1" to environment variables - Added "Install Dart" step using dart-lang/setup-dart action to: - cargo-test-linux - cargo-test-macos - cargo-test-windows This ensures the Dart language implementation tests will pass in CI.
Rustfmt fixes:
- Fixed formatting of method chaining with .context()
- Fixed long string literal formatting
- Fixed module ordering in tests/languages/main.rs
Clippy fixes in src/languages/dart.rs:
- Use EnvVars::var() instead of std::env::var() (disallowed method)
- Use writeln!() macro instead of push_str(&format!()) (format_push_string)
- Use inline format args {package} instead of {} (uninlined_format_args)
- Import std::fmt::Write trait for writeln! macro
Clippy fixes in tests/languages/dart.rs:
- Add #[ignore = "reason"] instead of #[ignore] // comment
- Add backticks around PUB_CACHE in documentation
- Allow needless_raw_string_hashes (indoc! macro compatibility)
All warnings resolved, CI checks should pass.
Testing confirmed that `dart --version` outputs to stdout, not stderr. This matches the behavior of other language implementations in prek (Lua, Go) which also only read from stdout. Changes: - Removed unnecessary combining of stdout and stderr - Simplified to read only from stdout like other implementations - Updated comment to reflect actual Dart behavior Verified with test: `dart --version 1>stdout.txt 2>stderr.txt` Result: Version info goes to stdout, stderr is empty.
Improvements based on Philosophy of Software Design principles and Lua reference: - Simplify version parsing by chaining .and_then() (reduces from 3 to 2 context layers) - Eliminate duplicate dart executable finding by passing it as parameter to helpers - Extract package_config setup to dedicated helper function for better modularity - Keep run() method cleaner and more focused All changes maintain the same functionality while improving code clarity and reducing duplication.
|
Still early WIP-haven't checked the parts around the missing test yet. I'd like to try this out with my Flutter repo so that's why I start this PR. Also, I'm using Claude code on web (which is providing credits until tomorrow) for much of the implementation. Plan to do a deeper cleanup and address more details tomorrow. Let me know if you have any early feedback or suggestions! |
|
I came across this repo thanks to the PR to Scoop. It's a great project, and I hope to contribute some features or fixes as I get more familiar with it! |
|
Thank you! The code looks good to me. However, since I'm not very familiar with the Dart ecosystem, I'll need a bit more time to review the details and make sure everything works as expected. |
Implements executable compilation for Dart hooks to match pre-commit's behavior
and align with Go/Lua implementations in prek.
Changes:
- Add compile_executables() helper to compile Dart executables from pubspec.yaml
- Update install_from_pubspec() to compile executables to env_path/bin after dart pub get
- Executables are compiled using 'dart compile exe' with proper platform handling (.exe on Windows)
- Run phase already compatible: env_path/bin is prepended to PATH, entry.resolve() finds binaries
Benefits:
- Faster hook execution (native binaries vs JIT compilation)
- Consistent with Go (go install) and Lua (luarocks) implementations
- Compatible with pre-commit's Dart language specification
- Supports both compiled hooks (remote with executables) and script hooks (local dart scripts)
Implementation details:
- Reads pubspec.yaml to discover executables
- Compiles each executable from bin/{name}.dart to env_path/bin/{name}[.exe]
- Gracefully handles missing executables or pubspec without executables section
- Uses PUB_CACHE environment variable for consistent dependency resolution
This test verifies that executables defined in pubspec.yaml are properly compiled to env_path/bin using 'dart compile exe'. The test was previously disabled with the note 'Requires complex setup with dart executables', but now that we've implemented dart compile exe support, it should pass in CI (which has Dart SDK installed). Test scenario: - Local hook with pubspec.yaml defining executables - Entry point is the executable name (not 'dart script.dart') - Expects the compiled binary to be found and executed
The previous implementation only checked repo_path() for pubspec.yaml, which worked for remote hooks but failed for local hooks where the pubspec.yaml is in work_dir. Problem: - Local hooks with pubspec.yaml defining executables were not compiled - Test 'with_pubspec_and_dependencies' failed with 'program not found' - The executable was never compiled because we skipped install_from_pubspec Solution: - Check both repo_path (for remote hooks) and work_dir (for local hooks) - Compile executables from whichever location has a pubspec.yaml - This aligns with how local hooks should work: the hook repository IS the work directory Test scenario that now works: - repo: local - pubspec.yaml in work_dir with executables defined - entry: hello-world-dart (expects compiled binary) - Now properly compiles to env_path/bin/hello-world-dart[.exe]
Implements full Dart language support following TDD approach:
dart pub getdart pub cache addLanguage features:
Closes #51