-
Notifications
You must be signed in to change notification settings - Fork 23
Remove Swift Lint and Use Swift-Format #339
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
Conversation
sbSteveK
left a comment
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.
Let's pull out the non-formatting related changes into a separate PR. Formatting shows a huge diff and we don't want to lose the point we made non-formatting changes to ci.yml.
Thanks, updated. |
| - name: Run Swift-Format | ||
| run: | | ||
| brew install swift-format | ||
| swift-format lint --recursive --strict . |
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.
Can we specify a swift-format version so that we could use the same version across the team.
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 question but unfortunately for swift-format, I don't think there is an easy way to specify a version. The only option I can think of is building from source, which will be a pain. We can do something like brew install swift-format@version, but as new versions will be released, brew won't keep the old versions.
Using the latest from brew should work as we can just do brew upgrade whenever new swift-format versions are released with any breaking changes.
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 problem with this is that we will need to do a reformatting PR as soon as new versions of Swift-format are released with incompatible changes but I'm not sure how often that would be.
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.
@xiazhvera Any concerns with this approach or are we good to ship it?
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.
No other concerns, good to go.
Issue #, if available:
#74
Description of changes:
Remove SwiftLint Formatter, it had the following problems
Use Swift-Format (Not Swift Format which is a different popular 3p tool). It has the following advantages
From Rust Fmt experience, I believe that an auto formatter is crucial so that we can ignore personal formatting preferences and just focus on coding.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.