Skip to content
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

Only run the pre-commit hook on changed files #3145

Merged

Conversation

drsooch
Copy link
Collaborator

@drsooch drsooch commented Sep 7, 2022

Using an external action to grab all changed files, and pass that to the pre-commit check to reduce false-negatives.

NOTES:

Alternatively, could pass the arguments: pre-commit ... --from-ref HEAD^ --to-ref HEAD to run the action on the last commit. However, will only capture a single commit, even if the branch received multiple commits on a push.

I'm not even sure this get-diff-action will even do what I expect.

TESTING NOTES:

  1. first commit with no haskell source changes, worked as expected. Skipped all the steps when there were no diffs.
  2. Committing a single haskell source change is matched.
  3. Diffs across multiple commits are captured (looks like the get-diff action compares the PR to main branch so this will always be the case)
  4. probably need to split the GIT_DIFF variable to pass files appropriately to pre-commit - done

This seems to work as expected I refactored a bit to skip the pre-commit hook if there are no hs files in the changelog. (not that it saves any time).

This should enable us to be green for pre-commit check

@drsooch drsooch force-pushed the pre-commit-workflow-on-changed-files-only branch 6 times, most recently from 9f49185 to 4d6bb71 Compare September 7, 2022 03:37
@drsooch drsooch force-pushed the pre-commit-workflow-on-changed-files-only branch 10 times, most recently from 167c928 to 8264e7c Compare September 8, 2022 01:22
@drsooch drsooch force-pushed the pre-commit-workflow-on-changed-files-only branch from 8264e7c to a8d928d Compare September 8, 2022 01:35
@drsooch drsooch marked this pull request as ready for review September 8, 2022 01:53
Copy link
Collaborator

@kokobd kokobd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you!

Copy link
Member

@Ailrun Ailrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does --from-ref origin/master (or similar using github.base_ref) work? If so, I think it might be better to use that for pr, and for merged commit, we can use HEAD^.

@drsooch
Copy link
Collaborator Author

drsooch commented Sep 8, 2022

Does --from-ref origin/master (or similar using github.base_ref) work? If so, I think it might be better to use that for pr, and for merged commit, we can use HEAD^.

I believe the current 3rd party GitHub action does a simple compare to the branch we are merging to.

I guess it could just get tricky with all possible enumerations of which commit to count towards the change list. Pre-commit itself even suggested the following: --from-ref HEAD^^^ --to-ref HEAD.

Really what I was going for is anything new, we want to make sure we try to keep formatted. And didn't want to try to cover everything.

@Ailrun
Copy link
Member

Ailrun commented Sep 9, 2022

I guess it could just get tricky with all possible enumerations of which commit to count towards the change list.

I mean, if pre-commit supports target branch names like origin/master for --from-ref, it's not really hard, is it? Or you mean that pre-commit doesn't support that?

I believe the current 3rd party GitHub action does a simple compare to the branch we are merging to.

IMO, if pre-commit already supports the feature, it'd be better not to introduce more dependency (which will be maintenance burden).

@drsooch
Copy link
Collaborator Author

drsooch commented Sep 12, 2022

I guess it could just get tricky with all possible enumerations of which commit to count towards the change list.

I mean, if pre-commit supports target branch names like origin/master for --from-ref, it's not really hard, is it? Or you mean that pre-commit doesn't support that?

The docs I was reading, didn't use a specific ref like origin/master it used --from-ref HEAD^^^ --to-ref HEAD. I can experiment with using a specific ref like master. I started reading up on trying to figure out which HEAD^^^ or HEAD^ to use and there were so many edge cases I didn't even bother trying something simple like origin/master

I believe the current 3rd party GitHub action does a simple compare to the branch we are merging to.

IMO, if pre-commit already supports the feature, it'd be better not to introduce more dependency (which will be maintenance burden).

Definitely agree, I will try some other experiments when I can

@pepeiborra pepeiborra enabled auto-merge (squash) September 18, 2022 07:44
@pepeiborra pepeiborra merged commit ff4f29f into haskell:master Sep 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants