Add discouraged_init opt-in rule that discourages direct initialization of certain types #1731

ornithocoder · 2017-08-01T10:02:22Z

Implements #1306, requested by @Noobish1.

Opt-in rule,
Implements UIDevice() and Bundle() by default,
Allows severity configuration (warning or error) via .swiftlint,
Allows overwriting types via .swiftlint.

SwiftLintBot · 2017-08-01T10:16:00Z

	1 Warning
⚠️	Permanently added the RSA host key for IP address ‘192.30.253.113’ to the list of known hosts.

	12 Messages
📖	Linting Aerial with this PR took 0.35s vs 0.33s on master (6% slower)
📖	Linting Alamofire with this PR took 2.46s vs 2.39s on master (2% slower)
📖	Linting Firefox with this PR took 10.55s vs 10.43s on master (1% slower)
📖	Linting Kickstarter with this PR took 15.71s vs 15.2s on master (3% slower)
📖	Linting Moya with this PR took 0.73s vs 0.72s on master (1% slower)
📖	Linting Nimble with this PR took 1.42s vs 1.39s on master (2% slower)
📖	Linting Quick with this PR took 0.45s vs 0.44s on master (2% slower)
📖	Linting Realm with this PR took 2.2s vs 2.15s on master (2% slower)
📖	Linting SourceKitten with this PR took 0.82s vs 0.77s on master (6% slower)
📖	Linting Sourcery with this PR took 3.64s vs 3.56s on master (2% slower)
📖	Linting Swift with this PR took 10.19s vs 10.09s on master (0% slower)
📖	Linting WordPress with this PR took 9.62s vs 9.53s on master (0% slower)

Generated by 🚫 Danger

codecov-io · 2017-08-01T10:36:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f822e1c). Click here to learn what that means.
The diff coverage is 91.04%.

@@            Coverage Diff            @@
##             master    #1731   +/-   ##
=========================================
  Coverage          ?   87.55%           
=========================================
  Files             ?      214           
  Lines             ?    10535           
  Branches          ?        0           
=========================================
  Hits              ?     9224           
  Misses            ?     1311           
  Partials          ?        0

Impacted Files	Coverage Δ
...rameworkTests/DiscouragedDirectInitRuleTests.swift	`100% <100%> (ø)`
...intFramework/Rules/DiscouragedDirectInitRule.swift	`100% <100%> (ø)`
...gurations/DiscouragedDirectInitConfiguration.swift	`68.42% <68.42%> (ø)`

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 f822e1c...1726573. Read the comment docs.

marcelofabri · 2017-08-01T11:37:32Z

Source/SwiftLintFramework/Rules/DiscouragedInitRule.swift

+            let offset = dictionary.nameOffset,
+            let name = dictionary.name,
+            kind == .call,
+            dictionary.bodyLength == 0,


so, this rule actually just works for inits without parameters, right?

In this case, I'd rename the rule and try to improve the description as well.

Precisely, it's meant to direct initialization only. I'll update the rule name and description.

marcelofabri · 2017-08-01T11:38:02Z

Source/SwiftLintFramework/Rules/DiscouragedInitRule.swift

+            let name = dictionary.name,
+            kind == .call,
+            dictionary.bodyLength == 0,
+            configuration.discouragedInits.contains(name)


what do you think about also checking for Bundle.init() for example?

Sure, I'll do that.

marcelofabri · 2017-08-01T11:38:36Z

Source/SwiftLintFramework/Rules/RuleConfigurations/DiscouragedInitConfiguration.swift

+        return severityConfiguration.severity
+    }
+
+    private(set) public var discouragedInits: [String]


what do you think about making it a Set<String>?

Much better! Changing it.

marcelofabri · 2017-08-01T11:40:37Z

Source/SwiftLintFramework/Rules/RuleConfigurations/DiscouragedInitConfiguration.swift

+    public var severityConfiguration = SeverityConfiguration(.warning)
+
+    public var consoleDescription: String {
+        return severityConfiguration.consoleDescription


You need to include all configuration in the description.
For example:

return "severity: \(severityConfiguration.consoleDescription), included: \(discouragedInits)"

Sorry about that. Updating it.

marcelofabri · 2017-08-01T11:42:08Z

Source/SwiftLintFramework/Rules/RuleConfigurations/DiscouragedInitConfiguration.swift

+        }
+
+        if let included = [String].array(of: configuration["included"]) {
+            discouragedInits += included


I think this should always override the default values. Doing that, we provide a way to opt-out from the default configuration if needed (for example, one might have a class Bundle in their project).

No problem. I'll update the code.

marcelofabri · 2017-08-01T11:42:50Z

Tests/SwiftLintFrameworkTests/RulesTests.swift

@@ -62,6 +62,26 @@ class RulesTests: XCTestCase {
        verifyRule(DiscardedNotificationCenterObserverRule.description)
    }

+    func testDiscouragedInit() {


can you add a new test class and split this in several tests, please?

Sure, but I might have to disable the type_body_length rule for this file, tho.

I meant making it a new file. Sorry, should have been more explicit about it.

Sure, no problem. Should I leave the "default" one in this one?

I think it makes sense to move everything to the new file. See FileHeaderRuleTests.swift for example.

I'll move them all. Easier to maintain ;-)

marcelofabri · 2017-08-01T11:44:11Z

CHANGELOG.md

@@ -92,6 +92,11 @@
  [Marcelo Fabri](https://github.com/marcelofabri)
  [#1714](https://github.com/realm/SwiftLint/issues/1714)

+* Add `discouraged_init` opt-in rule that discourages direct
+  initialization of certain classes.  


I'd use "types" instead of "classes" here.

@Noobish1

Rule requested by @Noobish1 on issue realm#1306.

marcelofabri · 2017-08-01T16:22:22Z

Source/SwiftLintFramework/Rules/RuleConfigurations/DiscouragedDirectInitConfiguration.swift

+            try severityConfiguration.apply(configuration: severityString)
+        }
+
+        if let included = [String].array(of: configuration["included"]) {


actually, on a second thought, what do you think about using "types" as the key? I'm afraid that we already overload the term included too much.

It makes sense. Changed.

marcelofabri · 2017-08-01T16:22:56Z

Tests/SwiftLintFrameworkTests/DiscouragedDirectInitRuleTests.swift

+class DiscouragedDirectInitRuleTests: XCTestCase {
+    let baseDescription = DiscouragedDirectInitRule.description
+
+    func testDiscouragedDirectInitRuleWithDefaultConfiguration() {


could you remove the Rule from the method names just to keep consistent with other tests?

Sure, removed.

marcelofabri · 2017-08-01T16:23:51Z

@ornithocoder This looks great! I just have two more small nitpicks 😅

jpsim · 2017-08-01T17:24:03Z

Why is this opt-in?

marcelofabri · 2017-08-01T17:22:02Z

Source/SwiftLintFramework/Rules/RuleConfigurations/DiscouragedDirectInitConfiguration.swift

@@ -46,7 +46,7 @@ public struct DiscouragedDirectInitConfiguration: RuleConfiguration, Equatable {
            try severityConfiguration.apply(configuration: severityString)
        }

-        if let included = [String].array(of: configuration["included"]) {
+        if let included = [String].array(of: configuration["types"]) {


You need to change on consoleDescription too

Yep, and the variable name :-) Done.

ornithocoder · 2017-08-01T20:20:27Z

@jpsim @marcelofabri I didn't spend much time thinking of that and went for the easy option. Should I make it a default rule instead?

marcelofabri · 2017-08-01T20:57:20Z

I think that the chance of getting a false positive with UIDevice or Bundle is low, so 👍 on making this enabled by default.

jpsim · 2017-08-01T21:19:49Z

@marcelofabri you merged this but it's still marked as opt-in... 🤔

did you mean to change that before merging?

marcelofabri · 2017-08-01T21:21:04Z

I'm opening a new PR to change it.

marcelofabri · 2017-08-01T21:22:21Z

Here it is: #1736. My plan was to merge it without a PR (🙈), but I've realized that actually LinuxMain.swift was outdated.

ornithocoder changed the title ~~Add discouraged_init opt-in rule that discourages direct (issue #1306)~~ Add discouraged_init opt-in rule that discourages direct Aug 1, 2017

ornithocoder changed the title ~~Add discouraged_init opt-in rule that discourages direct~~ Add discouraged_init opt-in rule that discourages direct initialization of certain types Aug 1, 2017

marcelofabri reviewed Aug 1, 2017

View reviewed changes

Add discouraged_direct_init opt-in rule

13c7e6e

Rule requested by @Noobish1 on issue realm#1306.

ornithocoder force-pushed the discourage_init branch from 35548b1 to 13c7e6e Compare August 1, 2017 15:24

marcelofabri reviewed Aug 1, 2017

View reviewed changes

Apply code review changes

b717780

marcelofabri reviewed Aug 1, 2017

View reviewed changes

ornithocoder added 2 commits August 1, 2017 19:29

More code review

a9cc8e5

Merge branch 'master' into discourage_init

1726573

marcelofabri merged commit d916dbb into realm:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add discouraged_init opt-in rule that discourages direct initialization of certain types #1731

Add discouraged_init opt-in rule that discourages direct initialization of certain types #1731

ornithocoder commented Aug 1, 2017 •

edited

Loading

SwiftLintBot commented Aug 1, 2017 •

edited

Loading

codecov-io commented Aug 1, 2017 •

edited

Loading

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

marcelofabri commented Aug 1, 2017

jpsim commented Aug 1, 2017

marcelofabri Aug 1, 2017

ornithocoder Aug 1, 2017

ornithocoder commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

jpsim commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

Add discouraged_init opt-in rule that discourages direct initialization of certain types #1731

Add discouraged_init opt-in rule that discourages direct initialization of certain types #1731

Conversation

ornithocoder commented Aug 1, 2017 • edited Loading

SwiftLintBot commented Aug 1, 2017 • edited Loading

codecov-io commented Aug 1, 2017 • edited Loading

Codecov Report

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

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

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 Aug 1, 2017

jpsim commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ornithocoder commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

jpsim commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

marcelofabri commented Aug 1, 2017

ornithocoder commented Aug 1, 2017 •

edited

Loading

SwiftLintBot commented Aug 1, 2017 •

edited

Loading

codecov-io commented Aug 1, 2017 •

edited

Loading