SwiftLint integration #249
Conversation
Package.swift
Outdated
| ], | ||
| dependencies: [ | ||
| .package(url: "https://github.com/kylef/PathKit.git", from: "0.9.0"), | ||
| .package(url: "https://github.com/kylef/Spectre.git", from: "0.9.0"), |
There was a problem hiding this comment.
Personally I actually like these trailing comma's. Leads to:
- cleaner git diffs
- less conflicts
- easier copy and pasting of lines
- easier reordering of lines
Thoughts?
There was a problem hiding this comment.
IMO we should avoid a bunch of copy/paste? More often than not, it leads to mistakes.
| let expectedError = expectedSyntaxError(token: token, template: template, description: reason) | ||
|
|
||
| let error = try expect( | ||
| self.environment.render(template: self.template, context: ["names": ["Bob", "Alice"], "name": "Bob"]), |
There was a problem hiding this comment.
extract context in a local variable and also move template and context parameters on separate lines?
There was a problem hiding this comment.
I'd prefer to keep this function in a single line though, same as expectation later.
There was a problem hiding this comment.
extract context in a local variable
Could you clarify this bit?
There was a problem hiding this comment.
let context = ["names": ["Bob", "Alice"], "name": "Bob"])
self.environment.render(template: template, context: context, file: file, line: line, function: function)
There was a problem hiding this comment.
Ah ok.
in a single line though
Problem is, and this in quite a few places, that we have absolutely gigantically long function calls/init/...
Thanks to swiftlint, we can enforce a hard limit on this, to improve readability. We can try keeping some things on one line, but here and there, we will have to split things up into multiple lines.
There was a problem hiding this comment.
yes, I agree, though having a function call that takes more than 5 lines probably is a sign that we should refactor it.
There was a problem hiding this comment.
True, but there's not much of a choice here, we're passing along parameters to another function 🤷♂️
| } | ||
|
|
||
| func testSyntaxError() throws { | ||
| template = Template(templateString: """ |
There was a problem hiding this comment.
I don't see profit from using multi-line strings in this and next function 🤷♂️
There was a problem hiding this comment.
It's to avoid having to escape " (quotes).
There was a problem hiding this comment.
I think escaping is fine... Don't we allow using single quotes though?
There was a problem hiding this comment.
Sure, we could use '.
But you'll see in some other tests I've placed the template in multiline strings just to keep the formatting consistent between multiple tests. It's a nice visual queue of "hey, that's a template", or "hey, that's an expected result".
djbe
force-pushed
the
feature/swiftlint
branch
from
75ef1c7
to
beec0e7
Compare
djbe
force-pushed
the
feature/swiftlint
branch
from
beec0e7
to
9243bba
Compare
|
Most of this PR is good to go, some of the feedback can't really be fixed without a more extensive refactor of the unit tests. I'm going to merge this for now, and further feedback/refactors can be done in other PRs. This one is an absolute hell to rebase after each PR 😱 |
Note: I recommend you view these changes either by checking the "Hide whitespace changes" in GitHub, or in a local git client and hiding whitespace changes.
This adds the
swiftlintconfiguration files to the project, and the necessary steps to CI.Sources/are pretty minimal, most done byswiftlint autocorrect, and the rest are tiny renames/moving code around.Tests/are quite extensive. Nothing really functional, more in the sense of reorganisation. The problem is that the old tests were all one (insanely) massive function per file, I've had to split these up into smaller functions. A remaining issue is that we may want to further split up the tests into smaller files.The advantage of the last item, is much nicer Xcode integration for some tests, as some tests are a single
testfunction now, showing up in the "Test Navigator". If the rest of the team agrees with this approach, I can apply it to the rest of the test cases. It's also a partial application of https://github.com/orgs/stencilproject/teams/coreteam/discussions/5.