Add options to disable some checks for identifier_name rule #1444

jaherhi · 2017-04-17T18:29:50Z

Add options mentioned in #541 for IdentifierNameRule, in order to allow names that start with an uppercase character and to exclude non alphanumeric characters.

jaherhi · 2017-04-17T18:31:18Z

Tests/SwiftLintFrameworkTests/IdentifierNameRuleTests.swift

+            "enum Foo { case MyEnum }"
+        ]
+        let triggeringExamples = baseDescription.triggeringExamples
+            .filter { !$0.contains("MyLet") && !$0.contains("MyEnum") }


Would it make sense to add triggering and non triggering examples to IdentifierNameRuleExamples for these two new tests even if they are only used in the tests?

I'm not sure what you mean here, could you elaborate?

I meant adding new static properties in IdentifierNameRuleExamples.swift (e.g. swift3NonTriggeringExamplesWithIgnoreStartWithLowercase or similar), instead of appending the two examples, let MyLet = 0 and enum Foo { case MyEnum }, to the non triggering examples here, and then having to remove examples that contain MyLet and MyEnum for the triggering examples.

SwiftLintBot · 2017-04-17T18:46:34Z

	12 Messages
📖	Linting WordPress with this PR took 15.02s vs 13.74s on master (9% slower)
📖	Linting Alamofire with this PR took 4.91s vs 3.15s on master (55% slower)
📖	Linting Firefox with this PR took 17.95s vs 13.7s on master (31% slower)
📖	Linting Kickstarter with this PR took 22.93s vs 18.98s on master (20% slower)
📖	Linting Moya with this PR took 0.47s vs 0.46s on master (2% slower)
📖	Linting Nimble with this PR took 2.08s vs 1.9s on master (9% slower)
📖	Linting Aerial with this PR took 0.67s vs 0.41s on master (63% slower)
📖	Linting Realm with this PR took 3.43s vs 3.19s on master (7% slower)
📖	Linting SourceKitten with this PR took 1.33s vs 1.24s on master (7% slower)
📖	Linting Sourcery with this PR took 2.82s vs 2.55s on master (10% slower)
📖	Linting Swift with this PR took 17.6s vs 14.4s on master (22% slower)
📖	Linting Quick with this PR took 0.64s vs 0.71s on master (9% faster)

Generated by 🚫 danger

marcelofabri · 2017-04-18T20:19:49Z

Source/SwiftLintFramework/Rules/IdentifierNameRule.swift

@@ -70,7 +72,8 @@ public struct IdentifierNameRule: ASTRule, ConfigurationProviderRule {
                }
            }

-            if kind != .varStatic && name.isViolatingCase && !name.isOperator {
+            if (!configuration.ignoresStartWithLowercase || isFunction) &&


maybe extract this condition (inside parenthesis) into a variable?

marcelofabri

You need to update LinuxMain.swift so IdentifierNameRuleTests is run on Linux

codecov-io · 2017-04-19T20:27:50Z

Codecov Report

Merging #1444 into master will increase coverage by 0.09%.
The diff coverage is 87.64%.

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   83.07%   83.16%   +0.09%     
==========================================
  Files         182      185       +3     
  Lines        9117     9275     +158     
==========================================
+ Hits         7574     7714     +140     
- Misses       1543     1561      +18

Impacted Files	Coverage Δ
Tests/SwiftLintFrameworkTests/RulesTests.swift	`71.81% <ø> (+0.17%)`	⬆️
Source/SwiftLintFramework/Rules/TypeNameRule.swift	`92.45% <100%> (+0.29%)`	⬆️
...iftLintFrameworkTests/RuleConfigurationTests.swift	`77.58% <100%> (+0.31%)`	⬆️
.../SwiftLintFramework/Rules/IdentifierNameRule.swift	`95% <100%> (+0.26%)`	⬆️
...SwiftLintFramework/Rules/GenericTypeNameRule.swift	`93.65% <100%> (+0.1%)`	⬆️
...k/Rules/RuleConfigurations/NameConfiguration.swift	`84.44% <71.42%> (-3.8%)`	⬇️
...ftLintFrameworkTests/IdentifierNameRuleTests.swift	`86.66% <86.66%> (ø)`
...ts/SwiftLintFrameworkTests/TypeNameRuleTests.swift	`87.75% <87.75%> (ø)`
...tLintFrameworkTests/GenericTypeNameRuleTests.swift	`88% <88%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6f9a85...d652cfe. Read the comment docs.

marcelofabri

This is good 💯

Just some comments 😬

marcelofabri · 2017-04-22T20:07:09Z

Source/SwiftLintFramework/Rules/RuleConfigurations/NameConfiguration.swift

@@ -17,6 +17,8 @@ public struct NameConfiguration: RuleConfiguration, Equatable {
    var minLength: SeverityLevelsConfiguration
    var maxLength: SeverityLevelsConfiguration
    var excluded: Set<String>
+    var allowedSymbols: Set<String>


this configuration is used on TypeNameRule and GenericTypeNameRule too. Maybe we should add support on those rules as well?

also, can you update consoleDescription to show all variables?

It wouldn't be difficult to add it to TypeNameRuleand GenericTypeNameRule, but do you think it'll be a common case? I can't remember seeing any code base with types or generic types with symbols

I was thinking more about keeping them consistente since they all use the same configuration type

marcelofabri · 2017-04-22T20:08:57Z

Source/SwiftLintFramework/Rules/RuleConfigurations/NameConfiguration.swift

@@ -17,6 +17,8 @@ public struct NameConfiguration: RuleConfiguration, Equatable {
    var minLength: SeverityLevelsConfiguration
    var maxLength: SeverityLevelsConfiguration
    var excluded: Set<String>
+    var allowedSymbols: Set<String>
+    var ignoresStartWithLowercase: Bool


what do you think about validatesStartWithLowercase (defaulting to true)?

I think it looks better than my option 😆 I'll change it!

marcelofabri · 2017-04-22T20:10:26Z

Source/SwiftLintFramework/Rules/IdentifierNameRule.swift

@@ -70,7 +72,9 @@ public struct IdentifierNameRule: ASTRule, ConfigurationProviderRule {
                }
            }

-            if kind != .varStatic && name.isViolatingCase && !name.isOperator {
+            let requiresCaseCheck = !configuration.ignoresStartWithLowercase || isFunction


why the isFunction check? 🤔

It made sense to me not to allow this option for functions, I thought there would be more cases where someone would want to disable this, in order to migrate code with enum cases that start with uppercase for example, than cases where someone would have functions starting with uppercase. Nonetheless, I wouldn't mind changing this if you think it'd be better 🙂

marcelofabri

💯

jpsim · 2017-05-15T18:52:35Z

Minor changes in 61e9b39

jaherhi · 2017-05-15T20:07:48Z

Thanks for the help and feedback, @marcelofabri 🙂

jaherhi commented Apr 17, 2017

View reviewed changes

marcelofabri reviewed Apr 18, 2017

View reviewed changes

marcelofabri requested changes Apr 18, 2017

View reviewed changes

marcelofabri requested changes Apr 22, 2017

View reviewed changes

jaherhi added 6 commits May 10, 2017 20:53

Add option to exclude non alphanumeric characters

93583f3

Add option to allow names to start with uppercase

fe916c1

Add changes to CHANGELOG.md

b2ecffb

Fix tests

3245fd8

Rename option to validates_start_lowercase

9d31eac

Add options to generic_type_name and type_name rules

d652cfe

marcelofabri approved these changes May 14, 2017

View reviewed changes

marcelofabri merged commit eec8935 into realm:master May 14, 2017

marcelofabri mentioned this pull request May 14, 2017

How to enable only the max_length of variable_name rule? #541

Closed

jaherhi deleted the identifier_name_allow_non_alphanumeric_and_uppercase_characters branch May 15, 2017 20:23

marcelofabri mentioned this pull request May 25, 2017

Allow '_' both in variable names and type names #628

Closed

SimplyDanny mentioned this pull request Feb 19, 2023

identifier_name issue didn't show warning #4767

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add options to disable some checks for identifier_name rule #1444

Add options to disable some checks for identifier_name rule #1444

jaherhi commented Apr 17, 2017

jaherhi Apr 17, 2017 •

edited

Loading

marcelofabri Apr 18, 2017

jaherhi Apr 18, 2017

SwiftLintBot commented Apr 17, 2017 •

edited

Loading

marcelofabri Apr 18, 2017

marcelofabri left a comment

codecov-io commented Apr 19, 2017 •

edited

Loading

marcelofabri left a comment

marcelofabri Apr 22, 2017

marcelofabri Apr 22, 2017

jaherhi Apr 23, 2017

marcelofabri Apr 30, 2017

marcelofabri Apr 22, 2017

jaherhi Apr 23, 2017

marcelofabri Apr 22, 2017

jaherhi Apr 23, 2017

marcelofabri left a comment

jpsim commented May 15, 2017

jaherhi commented May 15, 2017 •

edited

Loading

Add options to disable some checks for identifier_name rule #1444

Add options to disable some checks for identifier_name rule #1444

Conversation

jaherhi commented Apr 17, 2017

jaherhi Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SwiftLintBot commented Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

marcelofabri left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 19, 2017 • edited Loading

Codecov Report

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

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 left a comment

Choose a reason for hiding this comment

jpsim commented May 15, 2017

jaherhi commented May 15, 2017 • edited Loading

jaherhi Apr 17, 2017 •

edited

Loading

SwiftLintBot commented Apr 17, 2017 •

edited

Loading

codecov-io commented Apr 19, 2017 •

edited

Loading

jaherhi commented May 15, 2017 •

edited

Loading