Auto-format with swift-format #150
Conversation
There was a problem hiding this comment.
Thanks a lot for proposing this @DePasqualeOrg! However, I'd prefer to make style rules super minimal at this stage of the project. Can we work on something simpler, like what mlx-swift does?
.swift-format
Outdated
| "lineLength": 120, | ||
| "maximumBlankLines": 1, | ||
| "respectsExistingLineBreaks": true, | ||
| "lineBreakBeforeEachArgument": true, | ||
| "multiElementCollectionTrailingCommas": true, | ||
| "spacesAroundRangeFormationOperators": true, | ||
| "rules": { | ||
| "AlwaysUseLowerCamelCase": false | ||
| } |
There was a problem hiding this comment.
Not a fun of imposing too many rules given the experimental nature of this project. Something like what's used in mlx-swift is enough in my opinion, and it solves most of the distracting differences I've seen in the project so far.
There was a problem hiding this comment.
I've changed the rules to mirror those used in mlx-swift. This applies a default line length of 100, which in my opinion makes the code look less nice. Not setting lineBreakBeforeEachArgument and multiElementCollectionTrailingCommas could result in more distracting diffs.
There was a problem hiding this comment.
Ok, in that case let's use 120 for the line length, as you initially suggested, and multiElementCollectionTrailingCommas if you feel strongly about it. But I'd prefer to skip the lineBreakBeforeEachArgument for now, I find multi-lines more distracting than helpful when you are browsing the code.
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
I'd remove this file too to make it clear that git hooks are completely optional. Having a CI action to verify style is enough in my opinion. Contributors that want to auto-apply rules are still free to do so.
There was a problem hiding this comment.
I included it because it's included in mlx-swift and mlx-swift-examples, and pre-commit hooks are not required there either. It just makes it simple to run the formatting with pre-commit run --all-files instead of having to remember the swift-format command. It would also unify the workflow for contributors to all these packages, so that we just have to remember one command.
There was a problem hiding this comment.
All right, let's keep it then, I see it being useful for documentation and consistency.
.github/workflows/swift-format.yml
Outdated
There was a problem hiding this comment.
Thanks for coming up with this workflow! However, our CI configuration is a bit special for security reasons, we need to add workflows to this repo. We can work on a separate PR there!
There was a problem hiding this comment.
DePasqualeOrg
force-pushed
the
swift-format
branch
2 times, most recently
from
8978a09 to
47a424a
Compare
DePasqualeOrg
force-pushed
the
swift-format
branch
from
47a424a to
c862470
Compare
|
Done, thank you! |
I copied the swift-format and pre-commit configs that were recently added to Jinja. I also added a GitHub action to check formatting. This should minimize distracting diffs due to formatting. Feel free to change the formatting rules if you have a strong preference about them.
For future PRs, formatting can be applied with
pre-commit run --all-filesbefore committing.