variable_name rule should extend to all identifiers #954

marcelofabri · 2016-12-09T19:26:48Z

Fixes #663

I haven't applied all the rules to functions because I didn't think it was a good idea as their limits probably should be different.

This is a breaking change, I'm not sure how we should handle this. Perhaps we should keep a variable_name rule to avoid breaking configs?

codecov-io · 2016-12-09T20:18:47Z

Current coverage is 81.91% (diff: 74.31%)

Merging #954 into master will decrease coverage by <.01%

@@             master       #954   diff @@
==========================================
  Files           168        168          
  Lines          8344       8388    +44   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6835       6871    +36   
- Misses         1509       1517     +8   
  Partials          0          0

Powered by Codecov. Last update d3693e1...c8e0818

jpsim · 2016-12-12T01:33:18Z

Can you please rebase this so we can confirm it builds and passes tests on Linux?

marcelofabri · 2016-12-12T01:43:45Z

Tests/SwiftLintFrameworkTests/RulesTests.swift

@@ -397,6 +397,7 @@ extension RulesTests {
            // ("testForceUnwrapping", testForceUnwrapping),
            ("testFunctionBodyLength", testFunctionBodyLength),
            ("testFunctionParameterCountRule", testFunctionParameterCountRule),
+//            ("testIdentifierName", testIdentifierName),


I've commented it because testVariableName was commented too

That's fine for now. When we merge this, we should add this test to #966.

jpsim · 2016-12-12T01:53:08Z

CHANGELOG.md

@@ -7,6 +7,11 @@
  Guidelines but will shortly.  
  [JP Simard](https://github.com/jpsim),
  [Norio Nomura](https://github.com/norio-nomura)
+
+* `variable_name` rule is now `identifier_rule` as it validates other


identifier_name. Also, it'd be nice to keep VariableNameRule around, logging a warning to the console if used in comment commands or the config file, proxying its implementation to IdentifierNameRule. Would lead to a nicer deprecation path.

But wouldn't it make the rule run twice?

Might have to think this through a bit more. Maybe having rule ID aliases is the right move there.

I've created #973 to track this

marcelofabri · 2016-12-13T02:07:23Z

I've changed this to WIP as we need to figure out the deprecation part. However, I have no clue why the tests are failing on Linux 😭

marcelofabri · 2016-12-16T03:57:50Z

I was able to reproduce the crash on a local docker instance. It can be reduced to this:

let name = "matchPattern(_:excludingSyntaxKinds:excludingPattern:exclusionMapping:)"
let nameCharacterSet = CharacterSet(charactersIn: name)

Stacktrace:

* thread #1: tid = 40, 0x0000000000000000, name = 'repl_swift', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x0000000000000000
    frame #1: 0x00007ffff427fa8a libFoundation.so`_CFCharacterSetInitWithCharactersInString + 266
    frame #2: 0x00007ffff43a5736 libFoundation.so`function signature specialization <Arg[0] = Owned To Guaranteed and Exploded> of Foundation.NSCharacterSet.init (charactersIn : Swift.String) -> Foundation.NSCharacterSet + 150
    frame #3: 0x00007ffff43a3ace libFoundation.so`Foundation.NSCharacterSet.__allocating_init (charactersIn : Swift.String) -> Foundation.NSCharacterSet + 78
    frame #4: 0x00007ffff46493e5 libFoundation.so`Foundation.CharacterSet.init (charactersIn : Swift.String) -> Foundation.CharacterSet + 53
    frame #5: 0x00007ffff7f6dddf $__lldb_expr7`main + 127
    frame #6: 0x00000000004009b0 repl_swift`swift_once + 16

There are already two tickets on Swift bug tracking: SR-3215 and SR-3282.

It seems that the crash happens with any string with more then 63 characters.

jpsim · 2016-12-16T21:56:20Z

Can you just not include those examples in the rule description when compiling on Linux?

marcelofabri · 2016-12-16T22:58:11Z

Not really, because it's failing to lint a real file in IntegrationTests.testSwiftLintLints 😓

marcelofabri · 2016-12-18T22:39:57Z

A note to future self and all others: you can run lldb on a binary built with swift test:

Run docker interactively:

docker run --privileged -it -v \`pwd`:/SwiftLint norionomura/sourcekit:302

Run cd /SwiftLint && swift test to see it crashing
Run lldb .build/debug/SwiftLintPackageTests.xctest to start lldb
In lldb, type r to start the tests. When they crash, you can get a stack trace with bt.

jpsim · 2016-12-19T00:18:03Z

Source/SwiftLintFramework/Extensions/CharacterSet+LinuxHack.swift

+//  CharacterSet+LinuxHack.swift
+//  SwiftLint
+//
+//  Created by Marcelo Fabri on 17/12/16.


mm/dd/yy 🔥 🇺🇸

jpsim

I know this is still WIP (until we can handle the deprecation a bit better), but I have some comments in the meantime.

jpsim · 2016-12-19T00:27:17Z

Source/SwiftLintFramework/Extensions/CharacterSet+LinuxHack.swift

+            // workaround for https://bugs.swift.org/browse/SR-3215 and
+            // https://bugs.swift.org/browse/SR-3282
+            let sets = split(string, 63).map { CharacterSet(charactersIn: $0) }
+            let finalSet = sets.reduce(CharacterSet()) { (acc, set) -> CharacterSet in


This is a minor suggestion, but:

-let finalSet = sets.reduce(CharacterSet()) { (acc, set) -> CharacterSet in - acc.union(set) -} - -self = finalSet +self = sets.reduce(CharacterSet()) { final, set in final.union(set) }

jpsim · 2016-12-19T00:27:23Z

Source/SwiftLintFramework/Extensions/File+SwiftLint.swift

-    internal func validateVariableName(_ dictionary: [String: SourceKitRepresentable],
-                                       kind: SwiftDeclarationKind) -> (name: String, offset: Int)? {
+    internal func validateIdentifierName(_ dictionary: [String: SourceKitRepresentable],
+                                     kind: SwiftDeclarationKind) -> (name: String, offset: Int)? {


indentation

that was probably intentional to avoid breaking the return 😅
either way, I've moved this to the rule itself, since IMO it's not related to File in any obvious way.

jpsim · 2016-12-19T00:29:47Z

Source/SwiftLintFramework/Rules/IdentifierNameRule.swift

+    private func nameIsViolatingCase(_ name: String) -> Bool {
+        let secondIndex = name.characters.index(after: name.startIndex)
+        let firstCharacter = name.substring(to: secondIndex)
+        if firstCharacter.isUppercase() {


This might benefit from using guards:

-if firstCharacter.isUppercase() { - if name.characters.count > 1 { - let range = secondIndex..<name.characters.index(after: secondIndex) - let secondCharacter = name.substring(with: range) - return secondCharacter.isLowercase() - } - return true -} -return false +guard firstCharacter.isUppercase() else { return false } +guard name.characters.count > 1 else { return true } +let range = secondIndex..<name.characters.index(after: secondIndex) +let secondCharacter = name.substring(with: range) +return secondCharacter.isLowercase()

jpsim · 2016-12-19T00:31:45Z

Source/SwiftLintFramework/Rules/IdentifierNameRule.swift

+
+    private func isOperator(name: String) -> Bool {
+        let operators = ["/", "=", "-", "+", "!", "*", "|", "^", "~", "?", ".", "%", "<", ">", "&"]
+        return !operators.filter { name.hasPrefix($0) }.isEmpty


Prefer passing function references over simple closures:

-return !operators.filter { name.hasPrefix($0) }.isEmpty +return !operators.filter(name.hasPrefix).isEmpty

Nice catch! 💯

SwiftLintBot · 2016-12-26T18:56:40Z

	2 Warnings
⚠️	This PR introduced a violation in firefox-ios: /Client/Application/AppDelegate.swift#L288:13: warning: Switch Case on Newline Violation: Cases inside a switch should always be on a newline (switch_case_on_newline)
⚠️	This PR introduced a violation in firefox-ios: /Storage/Bookmarks/Trees.swift#L13:14: warning: Switch Case on Newline Violation: Cases inside a switch should always be on a newline (switch_case_on_newline)

	12 Messages
📖	Linting WordPress-iOS with this PR took 18.39s vs 16.04s on master (14% slower)
📖	Linting swift with this PR took 14.41s vs 16.02s on master (10% faster)
📖	Linting Aerial with this PR took 0.53s vs 0.53s on master (0% slower)
📖	Linting SourceKitten with this PR took 1.55s vs 1.67s on master (7% faster)
📖	Linting Sourcery with this PR took 3.13s vs 3.32s on master (5% faster)
📖	Linting ios-oss with this PR took 21.73s vs 21.39s on master (1% slower)
📖	Linting Alamofire with this PR took 3.92s vs 4.65s on master (15% faster)
📖	Linting firefox-ios with this PR took 26.05s vs 23.29s on master (11% slower)
📖	Linting Nimble with this PR took 2.98s vs 2.37s on master (25% slower)
📖	Linting Quick with this PR took 1.0s vs 0.72s on master (38% slower)
📖	Linting realm-cocoa with this PR took 4.25s vs 3.69s on master (15% slower)
📖	Linting Moya with this PR took 0.57s vs 0.54s on master (5% slower)

Generated by 🚫 danger

marcelofabri · 2016-12-30T04:15:16Z

Source/SwiftLintFramework/Rules/IdentifierNameRule.swift

+    private func validateName(_ dictionary: [String: SourceKitRepresentable],
+                              kind: SwiftDeclarationKind) -> (name: String, offset: Int)? {
+        let kinds = SwiftDeclarationKind.variableKinds() +
+            SwiftDeclarationKind.functionKinds() + [.enumelement]


this probably should check the Swift version to add or not .enumelement

jpsim · 2017-01-05T20:03:00Z

Maybe this rule shouldn't apply to @available(*, unavailable, renamed: ...) declarations since if they're acting as migration helpers from Swift 2 naming to Swift 3, they'll report violations. For example (as caught by osscheck!):

// MARK: - migration support
extension Request {
    @available(*, unavailable, renamed: "editorOpen(file:)")
    public static func EditorOpen(_: File) -> Request { fatalError() }
    ...
}

marcelofabri · 2017-01-05T20:10:11Z

Maybe this rule shouldn't apply to @available(*, unavailable, renamed: ...) ...

While I agree that it would be useful, I'm not convinced that it's worth the extra complexity. There's no way to get the unavailable attribute from AST and the offsets we have don't contain the @available attribute.

In both Realm and SourceKitten it'd be pretty easy to just ignore the rule in these declarations since they're isolated inside extensions.

jpsim · 2017-01-25T00:55:25Z

@marcelofabri what's next for this? Is this just blocked on me reviewing? Are there certain high-level points I should consider when reviewing this?

marcelofabri · 2017-01-25T00:59:52Z

If you're cool with not verifying the @available(*, unavailable, renamed: ...) annotation, this is ready for review.

I don't remember any important points, this is basically renaming the rule and adding more types to it. Also, some violations are skipped for functions. And this also validates that enum cases are lowercase on Swift 3 (I think there was an issue about it somewhere).

jpsim · 2017-02-09T20:07:40Z

CHANGELOG.md

@@ -939,7 +939,10 @@ This release has seen a phenomenal uptake in community contributions!

 ##### Enhancements

-* None.
+* `variable_name` rule is now `identifier_name` as it validates other


This is now in the wrong section

* master: (44 commits) make valid_docs rule opt-in update LineLengthConfiguration.consoleDescription after #1264 small refactoring after #1264 fix for_where violation in CompilerProtocolInitRule.swift Add for_where rule explicitly set podspec dependency versions Add changelog entry Fix existing violations Navigate substructure even for different kinds Small style changes Modified so that kinds(forByteOffset:) does not require a separate filter() setup of the results Changes from PR feedback. Long comments following code in a line will now trigger, configuration will now fail if invalud value types are set for options Fix false positive on large_tuple when using generics inside a tuple fix wording in changelog disable docs rules in Swift 2.3 and later Fix deadlock when stderr fills up OS buffer small Danger/oss-check improvements Fix links Fix cleanup Always use Xcode reporter on oss-check ...

jpsim · 2017-02-10T00:34:59Z

This definitely has a sizable impact on lint times, but that's to be expected. Hopefully with caching, no one feels the pain from this too much...

On my MacBook Pro:

Linting Alamofire with this PR took 2.01s vs 1.98s on master (1% slower)
Linting swift with this PR took 8.43s vs 6.84s on master (23% slower)
Linting Aerial with this PR took 0.31s vs 0.31s on master (0% slower)
Linting SourceKitten with this PR took 0.87s vs 0.83s on master (4% slower)
Linting Sourcery with this PR took 1.78s vs 1.49s on master (19% slower)
Linting ios-oss with this PR took 10.54s vs 10.21s on master (3% slower)
Linting Moya with this PR took 0.33s vs 0.29s on master (13% slower)
Linting firefox-ios with this PR took 12.21s vs 11.14s on master (9% slower)
Linting Nimble with this PR took 1.13s vs 1.14s on master (0% faster)
Linting Quick with this PR took 0.4s vs 0.36s on master (11% slower)
Linting realm-cocoa with this PR took 1.96s vs 1.7s on master (15% slower)
Linting WordPress-iOS with this PR took 6.78s vs 3.3s on master (105% slower)

marcelofabri commented Dec 12, 2016

View reviewed changes

jpsim requested changes Dec 12, 2016

View reviewed changes

marcelofabri mentioned this pull request Dec 13, 2016

Support rule aliases #973

Closed

marcelofabri changed the title ~~variable_name rule should extend to all identifiers~~ [WIP] variable_name rule should extend to all identifiers Dec 13, 2016

jpsim reviewed Dec 19, 2016

View reviewed changes

jpsim requested changes Dec 19, 2016

View reviewed changes

marcelofabri commented Dec 30, 2016

View reviewed changes

marcelofabri changed the title ~~[WIP] variable_name rule should extend to all identifiers~~ variable_name rule should extend to all identifiers Jan 4, 2017

marcelofabri mentioned this pull request Jan 4, 2017

TypeNameRuleExamples: added test to verify that enum cases are lowercase in Swift 3 #1115

Closed

marcelofabri and others added 11 commits January 23, 2017 21:38

Function kinds refactor

e1eae5e

Renaming enums

11364ed

VariableNameRule is now IdentifierNameRule

44c55f4

add missing trailing spaces to changelog entry

61c783f

Fixing changelog

dd384f5

Updating test

a150e26

Add workaround for CharacterSet crash on Linux

43a91ef

PR feedback

8715b91

Applying fix on Linux

8ef0703

Validate enum element only on Swift 3

5c42f59

Add deprecated alias to identifier_name rule

967c435

marcelofabri added 4 commits January 23, 2017 21:38

Swift 3 naming conventions

ec016a4

Updating rule identifier

7d94f5c

Fix changelog entry position

0f63224

Refactor to use new Dictionary methods

014531e

Merge branch 'master' into identifier-rule

c8e0818

jpsim approved these changes Feb 9, 2017

View reviewed changes

jpsim added 4 commits February 9, 2017 15:50

move changelog entry to appropriate section

337ec41

fix merge

fa3ab00

replace uses of 'variable_name' with 'identifier_name' in markdown files

a2ec02c

jpsim merged commit 944a989 into realm:master Feb 10, 2017

marcelofabri deleted the identifier-rule branch February 14, 2017 14:54

kimdv mentioned this pull request Mar 25, 2017

Add Rules to Support Ray Wenderlich's Swift Style Guide #319

Open

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

variable_name rule should extend to all identifiers #954

variable_name rule should extend to all identifiers #954

marcelofabri commented Dec 9, 2016

codecov-io commented Dec 9, 2016 •

edited

Loading

jpsim commented Dec 12, 2016

marcelofabri Dec 12, 2016

jpsim Dec 12, 2016

jpsim Dec 12, 2016

marcelofabri Dec 12, 2016

jpsim Dec 12, 2016

marcelofabri Dec 13, 2016

marcelofabri commented Dec 13, 2016

marcelofabri commented Dec 16, 2016 •

edited

Loading

jpsim commented Dec 16, 2016

marcelofabri commented Dec 16, 2016

marcelofabri commented Dec 18, 2016 •

edited

Loading

jpsim Dec 19, 2016

marcelofabri Dec 19, 2016

jpsim left a comment

jpsim Dec 19, 2016

marcelofabri Dec 19, 2016

jpsim Dec 19, 2016

marcelofabri Dec 19, 2016

jpsim Dec 19, 2016

jpsim Dec 19, 2016

marcelofabri Dec 19, 2016

SwiftLintBot commented Dec 26, 2016 •

edited

Loading

marcelofabri Dec 30, 2016

jpsim commented Jan 5, 2017

marcelofabri commented Jan 5, 2017 •

edited

Loading

jpsim commented Jan 25, 2017

marcelofabri commented Jan 25, 2017

jpsim Feb 9, 2017

jpsim commented Feb 10, 2017

variable_name rule should extend to all identifiers #954

variable_name rule should extend to all identifiers #954

Conversation

marcelofabri commented Dec 9, 2016

codecov-io commented Dec 9, 2016 • edited Loading

Current coverage is 81.91% (diff: 74.31%)

jpsim commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri commented Dec 13, 2016

marcelofabri commented Dec 16, 2016 • edited Loading

jpsim commented Dec 16, 2016

marcelofabri commented Dec 16, 2016

marcelofabri commented Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwiftLintBot commented Dec 26, 2016 • edited Loading

Choose a reason for hiding this comment

jpsim commented Jan 5, 2017

marcelofabri commented Jan 5, 2017 • edited Loading

jpsim commented Jan 25, 2017

marcelofabri commented Jan 25, 2017

Choose a reason for hiding this comment

jpsim commented Feb 10, 2017

codecov-io commented Dec 9, 2016 •

edited

Loading

marcelofabri commented Dec 16, 2016 •

edited

Loading

marcelofabri commented Dec 18, 2016 •

edited

Loading

SwiftLintBot commented Dec 26, 2016 •

edited

Loading

marcelofabri commented Jan 5, 2017 •

edited

Loading