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

[feat] add trunk fmt pre-push hook #801

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ryang20718
Copy link
Contributor

A user may have multiple commits they want to add in series. whilst trunk fmt is fast, it may not be instantaneous. would be nice to have an option to run prepush

Copy link

trunk-io bot commented Jun 7, 2024

⏱️ 4m total CI duration on this PR
Job Cumulative Duration Recent Runs
CodeQL-Build 2m 🟩
Trunk Check runner [linux] 1m 🟩
Repo Tests / Plugin Tests 38s 🟩
Action Tests 19s 🟩
Detect changed files 8s 🟩
Aggregate Test Results 1s 🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

@TylerJang27
Copy link
Collaborator

Hi @Ryang20718, thanks for the PR! I think that this action will not work as intended, however. GIT_INDEX_FILE is something that comes from git as an input to a pre-commit hook. trunk-fmt-pre-commit uses that to modify the index in place, so that the correct formatting of your files gets committed (and you don't have to run git commit again).

For a pre-push hook, there's no such construct. If you still want to add something like this, you could do the following with a wrapper:

  1. Check that the git workspace is clean
  2. Run trunk fmt (probably just with --trigger=pre-push)
  3. Add the changed files and commit them
  4. Verify that the correct ref is still being pushed

Let me know what you think, it's possible that there's also another option to smooth out your workflow; what formatter is usually the slowest to complete?

@Ryang20718
Copy link
Contributor Author

@TylerJang27 ready for a re-review!

Copy link
Collaborator

@TylerJang27 TylerJang27 left a comment

Choose a reason for hiding this comment

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

Can you add a comment explaining this action, since it's a slight departure from idiomatic usage?

@@ -18,6 +18,15 @@ actions:
- git_hooks: [pre-commit]
notify_on_error: false

- id: trunk-fmt-pre-push
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- id: trunk-fmt-pre-push
# We recommend using trunk-fmt-pre-commit in most cases, since it is
# generally pretty fast and modifies the index. When running this action
# via pre-push, note that it only reports unformatted files and doesn't modify
# them or modify the index. You may wish to use this action if you have a
# setup where formatters are too slow for pre-commit.
- id: trunk-fmt-pre-push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants