Run swiftformat via mint run #6261
Conversation
This avoids users needing to bootstrap manually and avoids permission problems with installation in /usr/local.
|
I expect |
scripts/style.sh
Outdated
| function swiftformat() { | ||
| mint run swiftformat "$@" | ||
| } | ||
|
|
||
| system=$(uname -s) | ||
| if [[ "$system" == "Darwin" ]]; then | ||
| version=$(swiftformat --version) |
There was a problem hiding this comment.
This (and the below) can probably be omitted as well, since the swiftformat version will be enforced by the Mintfile. Maybe instead they can be replaced by a check for a local Mint installation?
There was a problem hiding this comment.
I'd prefer to leave the version log in because it makes it easier to directly see in CI logs what happened. The comment about not checking the version is also useful because it contrasts with clang-format, which we do rigorously check.
There was a problem hiding this comment.
Actually, looking at the check failure this CL induced, the first run of mint run swiftformat installs it and that turns out to be this version check. It gives output like this:
Found: clang-format version 10.0.1
Found: 🌱 Cloning SwiftFormat 0.45.5
🌱 Resolving package
🌱 Building package
🌱 Installed SwiftFormat 0.45.5
🌱 Running swiftformat 0.45.5...
0.45.5
... which is a little ridiculous. I've removed the whole section now since this will be obvious enough.
scripts/style.sh
Outdated
| system=$(uname -s) | ||
| if [[ "$system" == "Darwin" ]]; then | ||
| version=$(swiftformat --version) | ||
| # Log the version in non-interactive use as it can be useful in travis logs. | ||
| # A version check is not requried because `mint bootstrap` ensures the |
README.md
Outdated
| @@ -137,16 +137,15 @@ See [HeadersImports.md](HeadersImports.md). | |||
| ### Code Formatting | |||
|
|
|||
| To ensure that the code is formatted consistently, run the script | |||
| [./scripts/style.sh](https://github.com/firebase/firebase-ios-sdk/blob/master/scripts/style.sh) | |||
| [./scripts/check.sh](https://github.com/firebase/firebase-ios-sdk/blob/master/scripts/check.sh) | |||
| before creating a PR. | |||
|
|
|||
| Travis will verify that any code changes are done in a style compliant way. Install | |||
There was a problem hiding this comment.
Done (though as GitHub Actions).
| brew install clang-format | ||
|
|
||
| clang-format -version |
There was a problem hiding this comment.
I think having the -version command for clang-format at least was helpful for debugging purposes - I imagine we can get rid of the swiftformat one since we know where it comes from at least. Any reason you removed the clang-format one otherwise?
There was a problem hiding this comment.
style.sh already logs the version used. This is duplicative.
scripts/style.sh
Outdated
| system=$(uname -s) | ||
| if [[ "$system" == "Darwin" ]]; then | ||
| version=$(swiftformat --version) | ||
| # Log the version in non-interactive use as it can be useful in travis logs. | ||
| # A version check is not requried because `mint bootstrap` ensures the |
scripts/style.sh
Outdated
| # /usr/local. | ||
| export MINT_PATH=Mint | ||
|
|
||
| function swiftformat() { |
There was a problem hiding this comment.
It looks like we only use this twice, and it may be less obvious if a developer goes to run this locally. Any objection to just changing the swiftformat to mint run swiftformat in the script below?
There was a problem hiding this comment.
No objection. Inlined.
Note that by hand use of swiftformat is not really something anyone can reasonably do. You need to pass a bunch of command-line options to control the style we use. Now you also need to set MINT_PATH. As a result, if you want to format a file without running all the checks you should just run style.sh.
There was a problem hiding this comment.
Yep that totally makes sense, I just realized that's exactly what I do.
The output of mint run includes details on which versions it is installing so there's no ambiguity in CI logs.
This avoids users needing to bootstrap manually and avoids permission problems with installation in /usr/local.