Changes for issue #598 to make line length rule configurable #1264

mlwelles · 2017-01-30T23:15:56Z

Hi, I took a pass at adding support for new new options on LineLengthRule to allow support for it to not trigger on long function declarations or long comments (or both): 'ignores_function_declarations' and 'ignores_comments', respectively.

…an options to LineLengthRule

…rule-configurable

SwiftLintBot · 2017-01-30T23:32:11Z

	12 Messages
📖	Linting WordPress-iOS with this PR took 14.46s vs 15.91s on master (9% faster)
📖	Linting swift with this PR took 13.79s vs 13.37s on master (3% slower)
📖	Linting Aerial with this PR took 0.5s vs 0.5s on master (0% slower)
📖	Linting SourceKitten with this PR took 1.4s vs 1.52s on master (7% faster)
📖	Linting Sourcery with this PR took 2.8s vs 2.87s on master (2% faster)
📖	Linting ios-oss with this PR took 19.67s vs 19.37s on master (1% slower)
📖	Linting Alamofire with this PR took 3.8s vs 3.72s on master (2% slower)
📖	Linting firefox-ios with this PR took 21.91s vs 21.75s on master (0% slower)
📖	Linting Nimble with this PR took 2.17s vs 2.26s on master (3% faster)
📖	Linting Quick with this PR took 0.66s vs 0.67s on master (1% faster)
📖	Linting realm-cocoa with this PR took 3.46s vs 3.44s on master (0% slower)
📖	Linting Moya with this PR took 0.55s vs 0.53s on master (3% slower)

Generated by 🚫 danger

ghost · 2017-01-31T08:31:57Z

Source/SwiftLintFramework/Rules/RuleConfigurations/LineLengthConfiguration.swift

@@ -8,34 +8,62 @@

 import Foundation

+public enum LineLengthConfigurationFlag {


Should this be an OptionSet?

Good suggestion. Changed it and updated the PR.

…er pull request feedback

mlwelles · 2017-02-01T20:41:21Z

It looks like the travis-ci job hung while checking the PR. There's no output I found indicating why the build could not be completed. I'll try and trigger it again with a dummy commit. Is there a better way?

marcelofabri · 2017-02-01T20:43:29Z

I'll trigger it manually

…I build -- it hung and errored after my last push

marcelofabri · 2017-02-02T22:22:52Z

Source/SwiftLintFramework/Extensions/Structure+SwiftLint.swift

@@ -17,17 +17,21 @@ extension Structure {
    /// - Parameter byteOffset: Int
    // swiftlint:disable:next valid_docs
    internal func kinds(forByteOffset byteOffset: Int) -> [(kind: String, byteRange: NSRange)] {
-        var results = [(kind: String, byteRange: NSRange)]()
+        return kinds().filter {


I expect this to be slower than the previous implementation because now we keep navigating in a substructure even if the top structure doesn't contain the range. Just a thing to keep in mind (i.e. we need to run oss-check locally to get reliable results about this one)

Yeah. I'll modify to have it all on a single pass.

marcelofabri

Thanks for implementing these!

I have added some comments. They're all small issues, but in general this looks great 🎉

marcelofabri · 2017-02-02T22:24:26Z

Source/SwiftLintFramework/Rules/LineLengthRule.swift

@@ -10,7 +10,7 @@ import Foundation
 import SourceKittenFramework

 public struct LineLengthRule: ConfigurationProviderRule, SourceKitFreeRule {


this shouldn't conform to SourceKitFreeRule anymore

marcelofabri · 2017-02-02T22:25:10Z

Source/SwiftLintFramework/Rules/RuleConfigurations/LineLengthConfiguration.swift


    var params: [RuleParameter<Int>] {
        return length.params
    }

-    public init(warning: Int, error: Int?, ignoresURLs: Bool) {
+    public init(warning: Int, error: Int?, options: LineLengthRuleOptions? = []) {


if we have a default value, I'd say it shouldn't be optional

marcelofabri · 2017-02-02T22:27:12Z

Source/SwiftLintFramework/Rules/LineLengthRule.swift

@@ -31,15 +31,37 @@ public struct LineLengthRule: ConfigurationProviderRule, SourceKitFreeRule {
    )

    public func validate(file: File) -> [StyleViolation] {
-        let minValue = configuration.params.map({ $0.value }).min(by: <)
+        let minValue = configuration.params.map({ $0.value }).min(by: <) ?? Int.max


I know this was already this way, but you can just use min() instead of min(by: <)

marcelofabri · 2017-02-02T22:30:30Z

Source/SwiftLintFramework/Rules/LineLengthRule.swift

+            }
+
+            if configuration.ignoresComments &&
+                line.index < syntaxKindsByLine.count {


this is almost identical to the implementation of the if before. What do you think about extracting to a private helper function?

Also, we probably should create a Set for SyntaxKind.commentKinds() and one for SwiftDeclarationKind.functionKinds() in private properties so we can get faster lookups (in theory anyway).

Done, updating PR

marcelofabri · 2017-02-02T22:31:01Z

Source/SwiftLintFramework/Rules/RuleConfigurations/LineLengthConfiguration.swift

+    public init(rawValue: Int) { self.rawValue = rawValue }
+    public init() { self.rawValue = 0 }
+
+    static let ignoreUrls = LineLengthRuleOptions(rawValue: 1 << 0)


these need to be public as well

done, updating PR

marcelofabri · 2017-02-02T22:36:31Z

Tests/SwiftLintFrameworkTests/LineLengthRuleTests.swift

@@ -12,10 +12,47 @@ import XCTest

 class LineLengthRuleTests: XCTestCase {

+    let longFunctionDeclaration = "public func superDuperLongFunctionDeclaration(a: String, b: String, " +


these variables should be private

marcelofabri · 2017-02-02T22:37:30Z

Tests/SwiftLintFrameworkTests/LineLengthRuleTests.swift

    func testLineLength() {
        verifyRule(LineLengthRule.description, commentDoesntViolate: false, stringDoesntViolate: false)
    }

+    func testLineLengthWithIgnoreFunctionDeclaraionsEnabled() {


typo in testLineLengthWithIgnoreFunctionDeclaraionsEnabled

marcelofabri · 2017-02-02T22:41:48Z

Source/SwiftLintFramework/Extensions/File+SwiftLint.swift

+        var maybeStructure = structureIterator.next()
+        while let line = maybeLine, let structure = maybeStructure {
+            if NSLocationInRange(structure.byteRange.location, line.byteRange) ||
+                NSLocationInRange(line.byteRange.location, structure.byteRange) {


do you really need these two NSLocationInRange checks? It seems redundant for me.

marcelofabri · 2017-02-02T22:42:54Z

CHANGELOG.md

@@ -23,6 +23,12 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#1061](https://github.com/realm/SwiftLint/issues/1061)

+* Add 'ignores_function_declarations' and 'ignores_comments' as options
+  to LineLengthRule.


missing two trailing spaces after .. Also, please scape ignores_function_declarations in backticks (`)

marcelofabri · 2017-02-02T22:43:07Z

CHANGELOG.md

@@ -1705,7 +1711,7 @@ This release has seen a phenomenal uptake in community contributions!
 * The following rules now conform to `ASTRule`:
  FunctionBodyLength, Nesting, TypeBodyLength, TypeName, VariableName.  
  [JP Simard](https://github.com/jpsim)
-
+  


unintentional change?

marcelofabri · 2017-02-03T00:33:36Z

Source/SwiftLintFramework/Rules/LineLengthRule.swift

+                let lineCommentKinds = syntaxKindsByLine[line.index].filter {
+                    return SyntaxKind.commentKinds().contains($0)
+                }
+                if !lineCommentKinds.isEmpty {


Actually, I don't know if this is the expected behavior. I'd expect that the line would be ignored if it only had comments.

A long line with a small comment in the end should trigger IMO.

… now trigger, configuration will now fail if invalud value types are set for options

filter() setup of the results

Small style changes after #1264

refactor and fix a few things after #1264

lukasburns · 2017-03-01T23:25:58Z

How do you use this option? any exampes, I would really appreciate it
thx!

sammy-SC · 2017-03-02T08:36:45Z

@lukasburns1 try

line_length:
  warning: 120
  ignores_function_declarations: true
  ignores_comments: true

However I believe this option is only available on Master and not in current release.

eomiso · 2022-01-11T18:39:21Z

Is this option now available in release? Tried them out but doesn't seem to work...

mlwelles added 3 commits January 30, 2017 17:51

Added "ignores_comments" and "ignores_function_declarations" as boole…

cd843e7

…an options to LineLengthRule

Merge branch 'master' of github.com:realm/SwiftLint into line-length-…

04437ed

…rule-configurable

Reverted debugging code

83a104d

This was referenced Jan 30, 2017

Make Line Length rule configurable #598

Closed

Add configuration to ignore function declaration on line_length rule #975

Closed

ghost reviewed Jan 31, 2017

View reviewed changes

Updated CHANGELOG.md

3f24204

mlwelles force-pushed the line-length-rule-configurable branch from aeaa2e6 to 3f24204 Compare January 31, 2017 18:23

Changed LineLengthConfigurationFlag enum to be an OptionSet struct, p…

ccc840b

…er pull request feedback

Added a middle initial to my name in the changelog to trigger a new C…

d5c8693

…I build -- it hung and errored after my last push

marcelofabri reviewed Feb 2, 2017

View reviewed changes

marcelofabri requested changes Feb 2, 2017

View reviewed changes

marcelofabri reviewed Feb 3, 2017

View reviewed changes

mlwelles added 3 commits February 6, 2017 15:33

Changes from PR feedback. Long comments following code in a line will…

c9ff0aa

… now trigger, configuration will now fail if invalud value types are set for options

fixed merge conflicts

7d48153

Modified so that kinds(forByteOffset:) does not require a separate

15a7a88

filter() setup of the results

marcelofabri approved these changes Feb 6, 2017

View reviewed changes

marcelofabri merged commit 56bbf12 into realm:master Feb 6, 2017

marcelofabri added a commit that referenced this pull request Feb 7, 2017

Merge pull request #1290 from marcelofabri/small-style

4ef01d1

Small style changes after #1264

mlwelles deleted the line-length-rule-configurable branch February 8, 2017 15:23

jpsim added a commit that referenced this pull request Feb 9, 2017

small refactoring after #1264

69f885c

jpsim added a commit that referenced this pull request Feb 9, 2017

update LineLengthConfiguration.consoleDescription after #1264

a10156b

jpsim added a commit that referenced this pull request Feb 9, 2017

Merge pull request #1300 from realm/jp-1264-fixes

811e11a

refactor and fix a few things after #1264

marcelofabri mentioned this pull request May 22, 2017

Add configuration for line_length to ignore comments #1541

Closed

ghost mentioned this pull request Jan 9, 2018

[JOSE-79] Improve AESEncrypter/Decrypter to be more generic airsidemobile/JOSESwift#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for issue #598 to make line length rule configurable #1264

Changes for issue #598 to make line length rule configurable #1264

mlwelles commented Jan 30, 2017

SwiftLintBot commented Jan 30, 2017 •

edited

Loading

ghost Jan 31, 2017

mlwelles Jan 31, 2017

mlwelles commented Feb 1, 2017

marcelofabri commented Feb 1, 2017

marcelofabri Feb 2, 2017

mlwelles Feb 6, 2017

marcelofabri left a comment

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

mlwelles Feb 6, 2017 •

edited

Loading

marcelofabri Feb 2, 2017

mlwelles Feb 6, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 2, 2017

marcelofabri Feb 3, 2017

lukasburns commented Mar 1, 2017

sammy-SC commented Mar 2, 2017

eomiso commented Jan 11, 2022

		@@ -8,34 +8,62 @@

		import Foundation

		public enum LineLengthConfigurationFlag {

		@@ -10,7 +10,7 @@ import Foundation
		import SourceKittenFramework

		public struct LineLengthRule: ConfigurationProviderRule, SourceKitFreeRule {

		@@ -12,10 +12,47 @@ import XCTest

		class LineLengthRuleTests: XCTestCase {

		let longFunctionDeclaration = "public func superDuperLongFunctionDeclaration(a: String, b: String, " +

Changes for issue #598 to make line length rule configurable #1264

Changes for issue #598 to make line length rule configurable #1264

Conversation

mlwelles commented Jan 30, 2017

SwiftLintBot commented Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlwelles commented Feb 1, 2017

marcelofabri commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelofabri 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

mlwelles Feb 6, 2017 • edited Loading

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

Choose a reason for hiding this comment

lukasburns commented Mar 1, 2017

sammy-SC commented Mar 2, 2017

eomiso commented Jan 11, 2022

SwiftLintBot commented Jan 30, 2017 •

edited

Loading

mlwelles Feb 6, 2017 •

edited

Loading