Multiline Brackets #2302
Multiline Brackets #2302
Conversation
|
Same issue here like in #2296, the |
|
@Dschee I wasn't able to review the PRs yet, but you're likely mixing UTF-8 and UTF-16 positions. SourceKit always works with UTF-8 byte offsets, so anything that you get from it (structure and syntax map) use that. Regex matching (and some NSString operations) use UTF-16 code units. |
Jeehut
changed the title
Jeehut
force-pushed
the
multiline_literal_brackets
branch
from
2fc347c to
33cc482
Compare
|
@marcelofabri Thanks for the pointer, it helped! I just fixed all issues and rebased this to the latest master. Looking forward to your review(s)! |
Codecov Report
@@ Coverage Diff @@
## master #2302 +/- ##
=========================================
+ Coverage 92.03% 92.1% +0.07%
=========================================
Files 302 305 +3
Lines 15180 15331 +151
=========================================
+ Hits 13971 14121 +150
- Misses 1209 1210 +1
Continue to review full report at Codecov.
|
| let range = file.contents.bridge().byteRangeToNSRange( | ||
| start: dictionary.bodyOffset!, | ||
| length: dictionary.bodyLength! | ||
| )! |
There was a problem hiding this comment.
How about using guard to make sure the bodyOffset, bodyLenght and range are not nil instead of force unwrapping them?
| ) -> [StyleViolation] { | ||
| var violations = [StyleViolation]() | ||
|
|
||
| if kind == .array || kind == .dictionary { |
There was a problem hiding this comment.
You can eliminate a nested level here by using guard for earlier return. The left hand margin of the code should be the "happy" path.
| ] | ||
| ) | ||
|
|
||
| public func validate( |
There was a problem hiding this comment.
For consistency with other rules, maybe this method signature coube be changed to:
public func validate(file: File,
kind: SwiftExpressionKind,
dictionary: [String: SourceKitRepresentable]) -> [StyleViolation]
| violations.append(StyleViolation( | ||
| ruleDescription: type(of: self).description, | ||
| severity: configuration.severity, | ||
| location: Location(file: file, byteOffset: dictionary.bodyOffset!) |
There was a problem hiding this comment.
Here you could use the safe bodyOffset from the guard I mentioned.
| violations.append(StyleViolation( | ||
| ruleDescription: type(of: self).description, | ||
| severity: configuration.severity, | ||
| location: Location(file: file, byteOffset: dictionary.bodyOffset! + dictionary.bodyLength!) |
There was a problem hiding this comment.
Here you could use the safe bodyOffset and bodyLength from the guard I mentioned.
CHANGELOG.md
Outdated
| @@ -23,6 +23,10 @@ | |||
| [Ornithologist Coder](https://github.com/ornithocoder) | |||
| [#2283](https://github.com/realm/SwiftLint/issues/2283) | |||
|
|
|||
| * Add new opt-in rule `multiline_literal_brackets` to warn against multiline | |||
| literal arrays & dictionaries with surrounding brackets without newline. | |||
| [Cihat Gündüz](https://github.com/Dschee) | |||
There was a problem hiding this comment.
Is there a ticket number (for reference, requisition/explaining why the rule is important and should be implemented)?
There was a problem hiding this comment.
There wasn't, but now I created an issue (#2306).
|
@ornithocoder Thank you for reviewing the PR, I just addressed all points, including creating an issue as reference to this rule request: #2306. I hope it looks good now! |
|
|
||
| private func openingBracketViolation(parameters: [[String: SourceKitRepresentable]], | ||
| file: File) -> StyleViolation? { | ||
| if |
There was a problem hiding this comment.
Here's another example where guard could be used for early return (of nil, in this case). Pretty much everything can be guarded in this method, leaving the return on the happy path.
guard
let firstParamByteOffset = ....,
let firstParamByteLength = ....,
...,
case let prefix = ...,
case let invalidPrefixRegex = ...,
let invalidMatch = invalidPrefixRegex.firstMatch(in: prefix, options: [], range: prefix.fullNSRange) else { return nil }
return StyleViolation()|
|
||
| private func closingBracketViolation(parameters: [[String: SourceKitRepresentable]], | ||
| file: File) -> StyleViolation? { | ||
| if |
|
I just addressed @ornithocoder's feedback and fixed a bug. |
|
@Dschee new rules are usually bigger PRs that can take a while for contributors to review. |
Jeehut
force-pushed
the
multiline_literal_brackets
branch
from
5e44877 to
aadf1a1
Compare
|
Just rebased to solve merge conflicts. |
|
@marcelofabri I see, makes sense. Will wait then and rebase from time to time. :) |
Jeehut
force-pushed
the
multiline_literal_brackets
branch
2 times, most recently
from
c6e6beb to
e11bac4
Compare
|
Just rebased and fixed an issue with missing CHANGELOG entries for two rules. |
Jeehut
force-pushed
the
multiline_literal_brackets
branch
from
e650cbb to
6422326
Compare
|
@marcelofabri Any estimate on when my new rules might be reviewed? I'm waiting over 7 weeks now without any updates, while during this time 6 other new rules were already merged: #2385, #2381, #2369, #2367, #2366, #2342 Here are my new rule suggestions, I'd appreciate your feedback: |
Jeehut
force-pushed
the
multiline_literal_brackets
branch
from
6422326 to
78c006d
Compare
|
Just rebased this branch onto current master. |
|
Please don't get me wrong, but I'm slowly starting to believe this project isn't actively maintained anymore. Apart from the new rule suggestions posted several months ago, I have also posted two bugfix PRs (#2426 and #2418) about 6 weeks ago without any comment. Both of them are very small changes with tests and CHANGELOG entries, so they could have been merged with a minute-long review and a click of a button, but they aren't. I know how time consuming maintaining open source software can be, so if not having enough time is the issue, I'd be happy to help as a maintainer of this repo if you accept me as such. We are really relying on an up-to-date linter to enforce code uniformity and code quality here at Jamit Labs in Karlsruhe, Germany and I would rather not start a new competing linter project if I don't have to. If this project however keeps to stay unmaintained like it is right now, I don't see any other way though. I highly appreciate all the work that was done until now, but I hope you understand that having SwiftLint configured to fail on our CI even if a single warning is shown really makes us less productive if we can't fix any false positives at the minimum (and add new autocorrectable rules at the optimum). |
Hi Cihat and first off, thank you very much for your significant contributions these last few months! I really appreciate it. Lots of people rely on SwiftLint and your efforts to improve their experience is impactful. I want to apologize for the reduced activity from maintainers to review and discuss issues or pull requests lately. This wasn't a conscious decision by anyone but likely more the people with write access collectively being busy with life and spending less time working on SwiftLint. Despite this, it's a stretch to call SwiftLint unmaintained, there have been 5 releases in the last 5 months, with over 80 changelog entries in that time, including some very significant new features like analyzer rules.
I just spent a few evenings going through my backlog of SwiftLint notifications and reviewed, updated and merged these two PRs and I can tell you it was significantly more involved than a minute-long review and a click of a button. The biggest contributor to SwiftLint's processing time is executing regular expressions, and the two involved in these PRs are particularly complex or expensive. Not only that, but there were unintended consequences to #2418, where that fixture was inadvertently picked up by Sourcery when generating our You did nothing wrong here, I could have made the exact same changes, but I'm just pointing out that reviewing community PRs may be more involved than you think.
That's fantastic! 🎉 I've been impressed with your contributions over the last few months. I just sent you an invite to gain write access to this project.
Lots of people use this project and rely on it being accurate, fast, comprehensive and actively maintained. Now that you have write access, hopefully you'll also help with ensuring this, collectively with other maintainers. ✊
💯 |
jpsim
force-pushed
the
multiline_literal_brackets
branch
from
78c006d to
ab13a2c
Compare
|
Regarding this PR specifically, looks good overall! I made some small refactors and I'll merge once CI is happy. |
|
Oh @jpsim, I just read your text today (I was on vacation last week) and thank you for the detailed explanation. I understand all your points (and actually hoped for this being the reasons behind my changed not being merged) – of cource even small PRs can be bigger than one might think at first. I just wanted to emphasize my point a little more. Thank you again, as I've stated elsewhere, for making me a maintainer. I will do what I can to help keeping SwiftLint as useful as possible. I agree that SwiftLint needs to stay fast, we didn't have any performance problems on our projects though, so I wasn't aware there existed any. But I will try to keep code running as fast as possible. |
Yeah, I don't think SwiftLint necessarily has performance problems for most projects. Running SwiftLint on itself lints 124 rules on 352 files in 2.9s on a clean lint and 300ms on a cached lint. However, running it on a large iOS app project with 5,000 Swift files takes 10x as long in both clean and cached cases, so that's where performance starts becoming something worth improving. |
* Add new multiline_literal_brackets rule with examples * Implement rule * Add changelog entry * Fix CHANGELOG and rule name * Fix tests + Update stuff after rebasing * Add more examples & fix whitespace issue * Address feedback from @ornithocoder * Add multiline rules for arguments and parameters * Fix false positives in rule multiline_parameters_brackets * Fix false positive for trailing closures in multiline_arguments_brackets * Add nested examples to rule multiline_arguments_brackets * Fix more false positives in multiline_arguments_brackets rule * Use guard where appropriate instead of if * Update generated artifacts after rebase * Add CHANGELOG entry for all three new rules * Move changelog entries to new version * Fix changelog entries position * Move new rules to correct subfolder * Update Rules.md file contents * Fixup changelog * Refactor rules
This fixes #2306.