-
Notifications
You must be signed in to change notification settings - Fork 6
pks integration #62
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
Open
martinemde
wants to merge
11
commits into
main
Choose a base branch
from
pks-integration-polish
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
pks integration #62
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Create a T::Struct that represents violations from pks JSON output with fields for violation_type, file, line, column, constant_name, and pack names. Includes methods for JSON deserialization and compatibility with the BasicReferenceOffense interface used by existing formatters.
Shells out to `pks check --output-format json <files>`, parses the response, and converts violations to PksOffense objects. Handles pks binary not found gracefully by raising PksBinaryNotFoundError.
Create JSON fixture files representing pks check output for various violation scenarios (privacy, dependency, multiple, strict mode). Extend pks_offense_spec with fixture-based tests to verify parsing. Add pks_wrapper_spec with test patterns ready for PksWrapper impl. All 90 existing tests continue to pass.
Replace PackwerkWrapper with PksWrapper to shell out to the pks binary for packwerk violation detection. This enables JSON-based violation parsing and prepares for removing the packwerk gem dependency. Changes: - Add PksWrapper to shell out to `pks check --output-format json` - Add Packwerk::ReferenceOffense adapter methods to PksOffense - Update DangerPackwerk#check to use PksWrapper.get_offenses_for_files - Update test mocks to stub PksWrapper instead of PackwerkWrapper
Replace all sorbet_double(Packwerk::ReferenceOffense, ...) mocks with real PksOffense objects in the test suite. This aligns the tests with the actual behavior of the PksWrapper, which returns PksOffense objects. Key changes: - Add build_pks_offense helper to spec_helper.rb - Replace Packwerk mocks in 'using inputted formatter' tests - Replace Packwerk mocks in 'using default formatter' tests - Update expectations where constant.location now points to the violation file instead of the constant definition file This is a known behavioral change: pks output doesn't include the constant definition location, so URLs and file paths in formatted messages now reference the violation file.
- Replace T.untyped with PackageAdapter in ConstantAdapter struct - Reorder adapter classes so PackageAdapter is defined before use - Convert multi-line if to modifier if in PksWrapper - Replace semicolons and $? with $CHILD_STATUS in specs
Document the pks (Rust packwerk) integration including: - Overview and benefits (10-100x faster than Ruby packwerk) - Installation options (dotslash and cargo install) - Migration notes (transparent, no code changes required) - JSON output format specification - Error handling for missing binary - Updated Advanced Usage to reference pks instead of bin/packwerk
Verify PksBinaryNotFoundError propagates correctly from PksWrapper through DangerPackwerk#check with a helpful error message directing users to install the pks binary.
2 tasks
Remove the adapter classes (PackageAdapter, ConstantAdapter, ReferenceAdapter, LocationAdapter) from PksOffense that existed solely to make it compatible with Packwerk::ReferenceOffense interface. Instead, use PksOffense's direct properties throughout: - offense.constant_name instead of offense.reference.constant.name - offense.file instead of offense.reference.relative_path - offense.line instead of offense.location.line - offense.defining_pack_name instead of offense.reference.constant.package.name Update Check::OffensesFormatter interface to accept PksOffense directly instead of Packwerk::ReferenceOffense, and update DefaultFormatter to resolve constant locations via Private.constant_resolver. Closes dp-5q5
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Replace ruby packwerk with pks. Uses the new json output from pks check to show packwerk offenses.
Why
pksis much faster