Add generic_type_name rule #1068
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marcelofabri
commented
Dec 25, 2016
| var ends = [Int]() | ||
| var currentOffset = 0 | ||
| let components = characters.split { character in | ||
| currentOffset += 1 |
There was a problem hiding this comment.
I'm pretty sure this wouldn't work with unicode characters 😅
There was a problem hiding this comment.
I've tried an alternative implementation but it's much slower 😱
fileprivate func split(separator: String) -> [(String, NSRange)] {
let nsText = self.bridge()
var index = 0
let length = nsText.length
var entries = [(String, NSRange)]()
while true {
let searchRange = NSRange(location: index, length: length - index)
let separatorRange = nsText.range(of: separator, options: [], range: searchRange)
if separatorRange.location == NSNotFound {
break
}
let previousIndex = index
index = NSMaxRange(separatorRange)
let range = NSRange(location: previousIndex, length: separatorRange.location - previousIndex)
let substring = nsText.substring(with: range)
entries.append((substring, range))
}
let range = NSRange(location: index, length: length - index)
let substring = nsText.substring(with: range)
entries.append((substring, range))
return entries
}
marcelofabri
commented
Dec 25, 2016
|
|
||
| fileprivate func trimmingWhitespaces() -> (String, NSRange) { | ||
| let range = NSRange(location: 0, length: bridge().length) | ||
| guard let match = regex("^\\s*(\\S*)\\s*$").firstMatch(in: self, options: [], range: range), |
There was a problem hiding this comment.
I hope there's a better way to do this 🤔
7 tasks
Current coverage is 82.69% (diff: 93.33%)@@ master #1068 diff @@
==========================================
Files 153 154 +1
Lines 7443 7578 +135
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 6141 6267 +126
- Misses 1302 1311 +9
Partials 0 0
|
jpsim
reviewed
Jan 4, 2017
| ) | ||
|
|
||
| private let genericTypePattern = "<(\\s*\\w.*?)>" | ||
| private var genericTypeRegex: NSRegularExpression { |
There was a problem hiding this comment.
Why isn't this private var genericTypeRegex = regex(genericTypePattern)?
There was a problem hiding this comment.
Because I wanted to make sure it's read-only and regex caches the objects anyway
There was a problem hiding this comment.
oh, because you can't initialize a variable with the contents with another one ¯_(ツ)_/¯
| private func validateGenericTypeAliases(_ file: File) -> [StyleViolation] { | ||
| let pattern = "typealias\\s+.+?" + genericTypePattern + "\\s*=" | ||
| return file.matchPattern(pattern).flatMap { (range, tokens) -> [(String, Int)] in | ||
| guard tokens.count > 1, tokens.first == .keyword, |
There was a problem hiding this comment.
I think the tokens.count > 1 check is superfluous and covered by the subsequent two checks.
| } | ||
|
|
||
| private func minParameterOffset(parameters: [[String: SourceKitRepresentable]], file: File) -> Int { | ||
| let offsets = parameters.flatMap { |
There was a problem hiding this comment.
these two flatMaps can be combined.
| file.contents.bridge().byteRangeToNSRange(start: $0, length: 0)?.location | ||
| } | ||
|
|
||
| return offsets.min() ?? Int.max |
| } | ||
|
|
||
| extension String { | ||
| fileprivate func split(separator: Character) -> [(String, NSRange)] { |
There was a problem hiding this comment.
Why did you make this function? Couldn't you use String.components(separatedBy:) instead? You should be able to get the ranges out of that too, since you know the length of the argument.
There was a problem hiding this comment.
I'd probably still have a helper function to calculate the ranges and return a [(String, NSRange)]. I could reimplement this function to use String.components(separatedBy:) instead. Do you think it would be better?
There was a problem hiding this comment.
you'd probably avoid the multi-byte character issue by doing so.
There was a problem hiding this comment.
🙌 I'll reimplement that
There was a problem hiding this comment.
Done in 9c6d900
jpsim
reviewed
Jan 5, 2017
|
|
||
| var previousEndOffset = 0 | ||
| let separatorLength = separator.bridge().length | ||
| return components.map { component -> (String, NSRange) in |
There was a problem hiding this comment.
map with mutation in defer? 😱
let separatorLength = separator.bridge().length
var previousEndOffset = 0
var result = [(String, NSRange)]()
for component in components(separatedBy: separator) {
let length = component.bridge().length
let range = NSRange(location: previousEndOffset, length: length)
result.append((component, range))
previousEndOffset += length + separatorLength
}
return result
|
This rule is crashing when linting Backtrace |
|
From the backtrace it seems the same issue as #1006, doesn't it? |
|
Yes, though none of the other rules hit it with that particular file. |
|
Is the file that you've linked the right one? Because the log says |
|
Yes, it's the right one. The log includes other files because we now lint multiple files in parallel. |
|
To be clear, I don't consider that issue to be a blocker for this PR, just sharing as an interesting discovery. |
jpsim
reviewed
Jan 5, 2017
| severity: severity, | ||
| location: Location(file: file, byteOffset: offset), | ||
| reason: "Generic type name should be between \(configuration.minLengthThreshold) and " + | ||
| "\(configuration.maxLengthThreshold) characters long: '\(name)'") |
There was a problem hiding this comment.
this was actually how Xcode formatted it 😱
Should I just align the quotes?
jpsim
reviewed
Jan 5, 2017
| } | ||
|
|
||
| fileprivate func trimmingWhitespaces() -> (String, NSRange) { | ||
| let range = NSRange(location: 0, length: bridge().length) |
There was a problem hiding this comment.
since you call bridge() twice in this function, could you instead just save it to a local let bridged = bridge() variable?
jpsim
approved these changes
Jan 5, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #51