-
Notifications
You must be signed in to change notification settings - Fork 9
Simplify pre-commit scripts. #439
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
Conversation
jorisdral
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! And it's also nice that it now prints the files that are checked/formatted
|
FYI, to make CI pass these lines have to be modified to call the renamed formatting scripts: lsm-tree/.github/workflows/haskell.yml Line 274 in c361efb
lsm-tree/.github/workflows/haskell.yml Line 212 in c361efb
|
262b27b to
b121090
Compare
mheinzel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
I've back ported some added robustness from a different project that checks for unstaged files and checks for the formatter versions. |
jorisdral
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM again. Let's squash the commits a bit before we merge
d86c2e9 to
aac59d2
Compare
|
Seems like something went wrong when rebasing, GitHub thinks you're adding 39 new commits. |
aac59d2 to
fecc54d
Compare
Caveats may apply.
Notably, this removes support for the path/command flags and requires that the scripts are run from the repository root.
I'm happy to restore support for the path/command flags and permit the commands to work from any directory, but figured I shouldn't complicate these files if that was unused complexity.