Inline test failure messages #3040

ZevEisenberg · 2020-01-15T04:00:06Z

Make it work.
Finish adding Example literal init to all strings and remove ExpressibleByStringLiteral conformance from Example type.
Find out whether we can remove trailing \n from the many test cases that have them.
~~Remove trailing newlines from all examples where possible.~~ rolled back because it was too invasive
Resolve file and line length warnings.
fix broken tests
~~merge https://github.com/ZevEisenberg/SwiftLint/pull/1~~ this PR is no longer dependent on that one

@jpsim here's a start! I took a pretty lazy approach to converting files over to the new format, opting for an ExpressibleByStringLiteral band-aid (that doesn't even partially work) just so I can get it up and running. Before merging, we'll need to go through and add an explicit Example(...) initializer to literally every example.

I've also been pretty liberal with smashing things in order to get it all compiling, so I'm very open to ideas if you have suggestions for how to make the PR more conservative.

I have two open questions that I'd love some feedback on:

A ton of examples have a trailing \n, but not all of them do. Do you know why it's there? Can I delete those newlines? Here's an example: "public func a() {}\n".
Some of the file and line length warnings don't seem that useful on a project like this, where file length isn't a good indicator of code complexity. it's just a bunch of "data"-like string literals, and sometimes the file gets longer just because we wrap lines to make them clearer. However, I'm not the project owner, so I'd appreciate any guidance you have on how to get around those warnings.

SwiftLintBot · 2020-01-15T04:50:15Z

	1 Warning
⚠️	Big PR

	12 Messages
📖	Linting Aerial with this PR took 1.2s vs 1.18s on master (1% slower)
📖	Linting Alamofire with this PR took 2.2s vs 2.16s on master (1% slower)
📖	Linting Firefox with this PR took 9.02s vs 8.98s on master (0% slower)
📖	Linting Kickstarter with this PR took 14.73s vs 14.63s on master (0% slower)
📖	Linting Moya with this PR took 1.14s vs 1.16s on master (1% faster)
📖	Linting Nimble with this PR took 1.36s vs 1.36s on master (0% slower)
📖	Linting Quick with this PR took 0.53s vs 0.56s on master (5% faster)
📖	Linting Realm with this PR took 2.35s vs 2.32s on master (1% slower)
📖	Linting SourceKitten with this PR took 1.0s vs 1.0s on master (0% slower)
📖	Linting Sourcery with this PR took 6.66s vs 6.66s on master (0% slower)
📖	Linting Swift with this PR took 12.26s vs 12.31s on master (0% faster)
📖	Linting WordPress with this PR took 14.88s vs 14.95s on master (0% faster)

Generated by 🚫 Danger

ZevEisenberg · 2020-01-15T05:01:02Z

It's early days, but here's a proof of concept. It's aliiiiiive!

jpsim · 2020-01-15T05:27:46Z

Thanks for your work on this!

A ton of examples have a trailing \n, but not all of them do. Do you know why it's there? Can I delete those newlines? Here's an example: "public func a() {}\n".

If I recall correctly, in the very early days all rules were run for every example, and one of the rules validated that files had a single newline at the end, so every example needed a newline to avoid triggering that rule. We now just run a single rule when validating each example, so these newlines are no longer required.

2. Some of the file and line length warnings don't seem that useful on a project like this, where file length isn't a good indicator of code complexity. it's just a bunch of "data"-like string literals, and sometimes the file gets longer just because we wrap lines to make them clearer. However, I'm not the project owner, so I'd appreciate any guidance you have on how to get around those warnings.

This is why examples are often split into separate files than the rule implementations. I'd argue that having that kind of data-like string literals mixed in with the implementation ends up being noisy and hard to read, even if it doesn't contribute to the complexity of the code. At the end of the day, the linter is just there to help. If the best thing for the code quality is to disable the rule in some cases, that's perfectly valid.

Finally, there's another big migration happening right now touching a ton of files: #3037
I just want to give you a heads up so you're not caught off guard if you invest time into this only to have many merge conflicts to then sort through.

ZevEisenberg · 2020-01-15T05:48:44Z

Thanks for the heads up on the other PR. Fortunately, they effectively merge cleanly right now (except for a file that I modified and you deleted; can I ask why some rule files were removed? Or is that temporary?).

jpsim · 2020-01-17T00:32:40Z

can I ask why some rule files were removed? Or is that temporary?

That was temporary as we were migrating rules. That PR has merged now and it looks like there are no conflicts with this one!

Source/SwiftLintFramework/Models/Example.swift

CHANGELOG.md

jpsim · 2020-01-17T00:36:47Z

CHANGELOG.md

-* None.
+* Inline test failure messages to make development of SwiftLint easier. Test failures in triggering and non-triggering examples will appear inline in their respective files so you can immediately see which cases are working and which are not.  
+  [ZevEisenberg](https://github.com/ZevEisenberg)
+  [#3040](https://github.com/realm/SwiftLint/pull/3040)


Can just drop this line completely: https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

I'm confused. That page says:

A list of Markdown hyperlinks to the issues the change addresses. One entry per line. Usually just one. If there was no issue tracking this change, you may instead link to the change's pull request.

Many other changelog entries link to the issue or PR. Why drop this line?

Source/SwiftLintFramework/Documentation/RuleDocumentation.swift

Source/SwiftLintFramework/Models/Example.swift

ZevEisenberg · 2020-01-17T03:11:02Z

@jpsim thanks for the comments. I know it’s a bit of a monster, so I’m glad to know it’s not going into the void! Removing the new lines may have been too ambitious, because it seems to have broken a couple of newline-sensitive rules in unexpected ways that I’m having a hard time pinning down, so I may back off on that.

I’m having a little trouble closing the last 5% of work, and I’d love to pair with you or someone else who has a lot of experience with the code base to get it over the finish line. I’m free this weekend and all of next week, so the timing is good for me (which is why I’m doing it in the first place 😅).

jpsim · 2020-01-17T04:21:03Z

I’m afraid I’m short on time these days but maybe you can find someone in the Slack group willing to help?

ZevEisenberg · 2020-01-17T18:45:22Z

I ended up removing the commits where I had deleted trailing new lines. Too much for one PR, and it was causing failures that I didn't want or need to deal with as part of this one.

ZevEisenberg · 2020-01-18T20:07:56Z

I was surprised how much fell into place once I had removed the \n-deleting commits. Now there are just three file/function body length failures, and I'd love your input on whether it would best be in the spirit of the project to try to fix them, or to disable the linter.

Also, since this is now ready for review, please let me know if there's anything I can do to make your job easier for reviewing this. I know it's a lot, so if I can help by wrangling reviewers, I'm happy to do so.

ZevEisenberg · 2020-01-18T22:01:49Z

@jpsim in order to fix the length warnings, I ended up restructuring the test helpers. There are no material changes here - just moving files and functions around. I figured you'd want to review that stuff separately: https://github.com/ZevEisenberg/SwiftLint/pull/1

PaulTaykalo · 2020-01-27T08:14:57Z

@ZevEisenberg please make sure that swift test is not failing on your machine
There are three errors ATM

/__w/1/s/Tests/SwiftLintFrameworkTests/TestHelpers.swift:455: error: IntegrationTests.testSwiftLintLints : failed - File should contain 400 lines or less: currently contains 455
/__w/1/s/Tests/SwiftLintFrameworkTests/TestHelpers.swift:164: error: IntegrationTests.testSwiftLintLints : failed - Function body should span 40 lines or less excluding comments and whitespace: currently spans 42 lines
/__w/1/s/Tests/SwiftLintFrameworkTests/TestHelpers.swift:370: error: IntegrationTests.testSwiftLintLints : failed - Function body should span 40 lines or less excluding comments and whitespace: currently spans 45 lines

ZevEisenberg · 2020-01-27T23:41:09Z

@PaulTaykalo thanks for taking a look. Fixing those failures involved a bunch of busywork that made this PR harder to review, so I fixed them on a sub-PR that I'd like to get reviewed and merged into this branch first. It's here, in case you've got time to look at it: https://github.com/ZevEisenberg/SwiftLint/pull/1

…ning in Xcode.

ZevEisenberg · 2020-02-01T20:25:13Z

Many thanks to @PaulTaykalo for helping me figure out the best way to get this branch ready to merge. I'm deferring the test helper cleanup, which will be an independent PR.

PaulTaykalo

few comments and I'm ok with merging it in

PaulTaykalo · 2020-02-01T22:39:32Z

Source/SwiftLintFramework/Models/StyleViolation.swift


    /// The severity of this violation.
-    public let severity: ViolationSeverity
+    public var severity: ViolationSeverity


any specific reason why do we need mutable severity and location

Personal style: I'm fine making things var if I need to change them, and letting the call site's use of let or var determine the mutability. But I'm also happy to add .with modifiers to the StyleViolation type 🙂

PaulTaykalo · 2020-02-01T22:59:52Z

Source/swiftlint/Helpers/LintOrAnalyzeCommand.swift

-                                      reason: $0.reason)
+                var new = $0
+                new.severity = .warning
+                return new


with(severity: .warning) ?

PaulTaykalo · 2020-02-01T23:03:26Z

Tests/SwiftLintFrameworkTests/TestHelpers.swift

-            return StyleViolation(ruleDescription: violation.ruleDescription, severity: violation.severity,
-                                  location: locationWithoutFile, reason: violation.reason)
+            var new = violation
+            new.location = locationWithoutFile


again we can use with(location: locationWithoutFile) extension
this will remove the need of having mutable property ad well as the some rules disabing
https://github.com/realm/SwiftLint/pull/3040/files#diff-32f83ec6a61cedc75c20b4319015fff9R6

jpsim · 2020-02-11T20:55:56Z

A big thank you @ZevEisenberg for pushing this through and @PaulTaykalo for the code review and shepherding this in! 👏

Sorry I haven't been able to help much.