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

Run SwiftFormat on commit #370

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

Patrick-Kladek
Copy link
Contributor

Description

To reduce the feedback loop for a failed build when formatting is not acceptable we agreed to run the formatter as part of the commit. Using https://github.com/hallettj/git-format-staged allows us to only format staged files, leaving other files untouched.

Tasks

  • Add pre commit hook
  • configure git hook on ./build-ios.sh

Infos for Reviewer

I tested this without SwiftFormat installed: The commit will fail with an error message explaining to install SwiftFormat. You still have the option to ignore the hook and commit directly.

Git, by default, does not distribute hooks when a repository is cloned. This design choice is intentional to prevent arbitrary code execution on users’ machines for security reasons. Therefore we need the workaround, where we configure the git hooks path as part for build-ios.sh

@Patrick-Kladek Patrick-Kladek marked this pull request as ready for review November 25, 2024 06:45
@ianthetechie ianthetechie self-requested a review November 25, 2024 06:58
Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

This looks good to me except for one thing.

FYI @Archdoog, I discussed the problem of how we (try to) push commits from CI that reform your code with the HudHud team last week, since you had complained about how it'd rewrite your branch causing local conflicts, and @ahmedre also ended up having an issue with this causing an inscrutable CI failure, since the action doesn't work on a cross-org fork.

Patrick suggested that we try this approach, which they have been using for 6+ months without issue. It's not as drastic as blanket running swiftformat (etc.) locally, and my understanding is that it only operates on staged files.

While I've been scarred by past experiences with git commit hooks from random JavaScript repos, I think this is a reasonable approach to try. If you're doing Swift development, there's like a 90%+ chance you already have swiftformat installed, and it doesn't use anything else outside git and the Python standard library.

Any objections? We can discuss in tomorrow's meeting if needed.

.githooks/pre-commit Outdated Show resolved Hide resolved
@Patrick-Kladek
Copy link
Contributor Author

I'm using https://fork.dev as my git client, so I can show some use cases:

Failure when swift file is committed and swiftformat is not installed

failure swiftformat not installed

Success after installing swiftformat

swift success

Android should be unaffected

java success

@Patrick-Kladek
Copy link
Contributor Author

@ianthetechie could this cause any issue with Windows? for a long time windows didn't support bash but that changed in recent years.

Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

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

We'll just merge this for now and if it causes any issues with a Windows user, we'll cross that bridge when we come to it :)

@ianthetechie ianthetechie merged commit 43391a3 into stadiamaps:main Nov 26, 2024
14 checks passed
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.

2 participants