-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add SwiftLint support #57
Add SwiftLint support #57
Conversation
defer { | ||
next() | ||
} | ||
do { |
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.
Turns out errors are automatically caught by Kitura: Kitura/Kitura@42eb6bf
This means we can throw wherever we like, and catch these errors and do something with them in a general-purpose error handling middleware. This is slated for #58
I've confirmed that the behaviour is the same for routes that throw.
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.
Much nicer! 👍 as long as we're logging the Error
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.
Alas, we aren't yet. That's what the referenced issue (#58) is for. Is not logging a blocker?
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.
Nah, the issue is tagged appropriately so we Gucci
} | ||
|
||
/// Intended for use by GitHub webhooks | ||
router.post("/api/refresh_workshops") { request, response, _ in | ||
try! WorkshopManager.update() |
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.
Willing to discuss removing the force try here, in which case we can leave it out of the PR and come back to the idea later.
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.
So am I right in thinking that with the force try, it would crash if WorkshopManager
crashes, and without it, it would use a potentially out of date set of workshops?
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.
Correct. We can add logging here and/or tests on the workshop manager repo to catch this stuff. I'm all for failing fast, but in this case an update to a GitHub repo can crash our app automatically. That's some spooky action at a distance right there.
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.
👍
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.
Looks good. Not sure how valuable this is without supporting it in the container. If our Windows contributors can't use these style guides then we'll certainly end up with the guides not stuck to, what are the limitations of the Linux setup?
- Sources | ||
excluded: | ||
- .build | ||
disabled_rules: |
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.
I find it kind of spooky that SwiftLint examples encourage opting out of rules you don't want rather than opting in to the ones you want. What are your thoughts on using enabled_rules instead?
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.
I'm happy to go down that direction, but I trust that SwiftLint have sensible defaults here. I'd much rather run into frustration for an overly restrictive rule and disable it, as opposed to missing out on a potentially useful rule.
Coming from JS linting land, maintaining a blacklist was much easier than maintaining a whitelist.
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.
That's fair. I suppose with pinning we're also not really in danger of new rules sneaking in without action from us. Let's stick to blacklist.
@@ -1,5 +1,7 @@ | |||
import HaCTML | |||
|
|||
// swiftlint:disable line_length |
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.
Seems like we might have to disable this for most ViewModels 🤷♀️
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.
Yeah I think so
Alternatively we can try to keep our line lengths shorter 😉
@jaredkhan no limitations on the Linux setup, just takes some time for us to put the docker config together (need to install SourceKit, set up build environment for SwiftLint, add it to path), then our Windows users can use it. Definitely something we want to do, just testing the waters atm. |
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 wait until Charlie's changes go in to merge this |
This PR adds support for SwiftLint, configures some rules that were particularly grating, and fixes all of the offenders in our current codebase.
Caveats:
swiftlint autocorrect
feature but will leave many rules for manual fixingI also did a teeny weeny bit of refactoring (only a little bit).