-
Notifications
You must be signed in to change notification settings - Fork 228
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 pre-commit hooks #712
base: main
Are you sure you want to change the base?
Conversation
df58814
to
756fbb3
Compare
I'm test-driving the hook from this MR in one of my projects using this configuration: repos:
- repo: https://github.com/danthorpe/swift-format
rev: danthorpe/add_pre_commit_hooks
hooks:
- id: swift-format There are two things I noticed:
In any case, happy to see this moving forward! 😊 |
Thanks for the comments @Feuermurmel - I wasn't familiar with the |
👋 @ahoppen @MaxDesiatov any update on this one? Would be really handy to get merged so that people can use swift-format in their pre-commit hooks. 🙏 |
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.
Sorry for taking so long to get back to you on this @danthorpe.
Thanks you for providing the integration. The change looks good to me. Just a very minor comments:
- Could you add a trailing newline to the YAML file?
- Could you rebase your changes so they are a single commit without a merge from
main
into your branch? That generally makes for a nicer Git history
Also, pushing changes should trigger the new GitHub Actions CI, which is required to merge the changes.
feba87c
to
c097787
Compare
c097787
to
a45febf
Compare
@ahoppen - done, thanks! 🎉 |
.pre-commit-hooks.yaml
Outdated
language: swift | ||
types: [swift] | ||
require_serial: true | ||
|
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.
The trailing newline has two spaces, could you remove them to make the YAML lint pass?
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.
Okay, done! 🤞
a45febf
to
2833b73
Compare
.pre-commit-hooks.yaml
Outdated
language: swift | ||
types: [swift] | ||
require_serial: true | ||
|
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.
It looks like the file still has a space here on the last line (GitHub is showing the indicator that the file doesn’t have a trailing newline).
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.
Okay, yep I've removed the space. Not sure if there is a new trailing line now though.
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.
Great. Looks good now.
2833b73
to
620f80b
Compare
@danthorpe Your changes look good, I’m investigating the unrelated CI failures. |
I took a shot at adding a pre-commit hooks files, according to this guide.
Fixes #676.
Once this is merged, it can be used in your other swift packages, by adding the following to the
.pre-commit-config.yaml
under therepos
section, according to the guide.