Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ExplicitInitRule #715

Closed
wants to merge 2 commits into from
Closed

Add ExplicitInitRule #715

wants to merge 2 commits into from

Conversation

mtaube
Copy link

@mtaube mtaube commented Jul 3, 2016

Hey guys, I added a quick rule to discourage the explicit use of init (#672). The current implementation only catches inits that are prefixed by a well-formed class / struct name.

// bad
let abc = Abc.init()

// good
let abc = Abc()

@codecov-io
Copy link

codecov-io commented Jul 3, 2016

Current coverage is 88.64% (diff: 100%)

No coverage report found for master at ed73db3.

Powered by Codecov. Last update ed73db3...5f27267

@jpsim
Copy link
Collaborator

jpsim commented Jul 8, 2016

Do people really write Abc.init() superfluously? Since there are legitimate uses (e.g. optionalString.flatMap(NSURL.init(string:))) that would trigger this warning, I'd be inclined to not add this rule.

@mtaube
Copy link
Author

mtaube commented Jul 8, 2016

I've seen some people at work do it, especially developers migrating from Obj-C. Also I think it might be because of Xcode autocomplete combined with laziness. Which is why I want to enforce with this rule on everybody.

What do you think about:

  1. Making it opt-in
  2. Creating a blacklist for the rule to allow for the .init, like the NSURL. I can't think of other cases where this would trigger false positives.

@jpsim
Copy link
Collaborator

jpsim commented Jul 8, 2016

We could make this opt-in. I'm generally more ok with false positives with opt-in rules.

Don't special case NSURL, there's nothing unique about it when it comes to using its initializer as a function reference.

@mtaube
Copy link
Author

mtaube commented Jul 25, 2016

@jpsim made this rule opt-in / correctable, lmk what you think

@@ -10,6 +10,10 @@
[bootstraponline](https://github.com/bootstraponline)
[#689](https://github.com/realm/SwiftLint/issues/689)

* Add ExplicitInitRule.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing two trailing spaces according to the guidelines at https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes

@jpsim
Copy link
Collaborator

jpsim commented Aug 21, 2016

I'm still pretty dubious of the value of such a rule, but since it's opt-in and if some find it useful, then why not.

@mtaube please address my latest round of comments and rebase this so I can review again.

@mtaube
Copy link
Author

mtaube commented Sep 20, 2016

@jpsim made those changes. Lmk if anything else is needed. Thanks for SwiftLint / Realm by the way – we use both at work.

@norio-nomura
Copy link
Collaborator

Thanks for doing this! 🙏
I have rewritten this as ASTRule for reducing false positive on #826, and merged that.

@mtaube mtaube deleted the mt-explicit-init branch October 30, 2016 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants