-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
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.
Overall OK, but would be worth addressing the SwiftLint version change + simplify the shell script if possible.
Which would make you do a new commit… forcing Travis to unstuck itself, hopefully ^^
Scripts/install_swiftlint.sh
Outdated
set -e | ||
|
||
SWIFTLINT_PKG_PATH="/tmp/SwiftLint.pkg" | ||
SWIFTLINT_PKG_URL="https://github.com/realm/SwiftLint/releases/download/0.9.1/SwiftLint.pkg" |
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.
We're at SwiftLint 0.14 now. instead of having to update the number every time a new version of SwiftLint is released I think GitHub exposes an URL for the latest release, maybe we can use that
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.
Note that I think I borrowed that script from SwiftLint or SourceKitten or somewhere like this, so maybe:
- Worth mentioning the source somewhere
- Worth checking if they have an updated version of this shell script
Scripts/swiftlint-output.sh
Outdated
# Lint generated code in Tests/Expected | ||
for f in `find "Tests/Expected" -name '*.swift'` | ||
do | ||
cat $f | swiftlint lint --use-stdin | sed s:'<nopath>':"$f": |
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 remember that I had to do this dance with stdin
because swiftlint
didn't accept files not ending in *.swift
So basically if I did swiftlint lint Tests/Expected/foo.swift.out
then swiftlint
would ignore the input file (because it didn't have a .swift
extension), hence the cat
+ --use-stdin
trick… + the replacement of <nopath>
then.
Now that the files in Tests/Expected
have been renamed to *.swift
we should be able to directly use swiftlint lint $f
I think. Or maybe even use find "Tests/Expected" -name '*.swift' -0 | xcargs -0 swiftlint lint
?
Seems there are 9 SwiftLint violations (line_length) in the test code itself, could be worth fixing before we merge this. |
b6e4ced
to
b7a17fc
Compare
Fixes #16