-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fixed returns doc for init methods issue-557 #739
Conversation
Current coverage is 89.39% (diff: 50.00%)@@ master #739 diff @@
==========================================
Files 77 77
Lines 2633 2640 +7
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 2357 2360 +3
- Misses 276 280 +4
Partials 0 0
|
@@ -51,7 +51,9 @@ func declarationReturns(declaration: String, kind: SwiftDeclarationKind? = nil) | |||
guard let outsideBracesMatch = matchOutsideBraces(declaration) else { | |||
return false | |||
} | |||
|
|||
if declaration.containsString("init(") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is dangerous, Consider a declaration of sorts:
actionPerformFinit()
This would still match this case incorrectly.
I think something like this would be better for this "Simple" style solution but I'm still not totally sure if checking for a string match is the best solution regardless. I think it could work for now if @jpsim thinks this is proper.
Even though this might break for required init
or override init
?
guard !declaration.hasPrefix("init(") else {
return true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@freak4pc You are right, should've considered the constant string more carefully.
- I'm not that familiar with the project to be honest, and I wasn't aware that the
declaration
string would be something like:public convince init()
or plaininit()
, that's why I didn't go for thehasPrefix
, but I see your point and I think a leading space could help! - I decided against the
guard
usage instead ofif
because the guidelines I'm following, prohibits usingguard
s for case testing and this is clearly testing against different scenarios, but, if SwiftLint suggests I should useguard
, by all means! - The very next line of code uses the same technique for checking if a function returns (by checking the existence of
->
), granted, that is very much unique in Swift's grammar, but then again so isinit()
, don't you agree? And with this logic what would become of a function with the following syntax:func runAndExit() -> Never { /* ... */ }
in Swift 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the beginning space solution should be good :)
I'm also not familiar enough with SourceKitten so im not sure if includes the ACL etc but it should be considered in my opinion.
About guard/if, As far as I know, the guidelines SwiftLint follows, do suggest using guard to return early.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the guideline suggestion. That's very helpful. Wilco. :)
This wouldn't work for failable initializers ( |
@jpsim Agree, I tried this solution but
returns |
Ah, interesting. That may be resolved later in the lexer than what's available to SourceKit. @mohpor could you at least update this to support both |
@jpsim Thanks for the heads up. The point here IMHO is that, a P.S.: I tried using regex to match the initializer syntax but I failed miserably! func declarationIsContructor(declaration: String) -> Bool {
return regex("\\sinit\\?*\\(.*\\)").numberOfMatchesInString(declaration, options: [], range: NSRange(location: 0, length: declaration.characters.count)) > 0
} But I couldn't make it work! It wouldn't match |
About your whitespace issue, ;-) |
The regex you've mentioned would match P.S.: Strangely enough, the regex I have provided before works in playground but fails in action! I will take a closer look at this. P.P.S: thanks a lot for guiding me through that vicious trailing white space, it's been bugging me for too long. 👍 |
The correct regex I think is something like this: |
Worked like a charm! |
@jpsim do you need anything to add to this one? |
It's been 8 days! Any news? |
@mohpor Could you make optional |
@Grubas7 I have updated the CHANGELOG.md. |
@mohpor Yeap, you're right. I forgot about this option on GitHub :) |
@@ -20,11 +20,15 @@ | |||
[bootstraponline](https://github.com/bootstraponline) | |||
[#689](https://github.com/realm/SwiftLint/issues/689) | |||
|
|||
* Made `- returns:` doc optional for constructors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing two trailing spaces as indicated in https://github.com/realm/SwiftLint/blob/master/CONTRIBUTING.md#tracking-changes
I'd also prefer if you replaced the word "constructors" with "initializers" because constructors also include the family of static functions that return Self
and this doesn't apply to that.
@mohpor thanks for your work on this, and sorry for the delay on reviewing this. I just had a few minor comments. Could you please address them, then I think this can be merged. |
@jpsim Done. P.S.: My branch has conflicts now. Should I do anything? |
@mohpor you'll probably have to rebase against master |
@freak4pc Thanks again. You're the man. |
You don't need to close this PR, you can rebase (correctly) and force-push to the same branch. |
Signed-off-by: M. Porooshani <porooshani@gmail.com>
Anytime. Thanks for your consistent help on this PR! @mohpor |
🎉 congrats for landing this @mohpor |
This bug is still valid for implicitly unwrapped optional, eg: |
huh, forgot it was possible to make initializers like that. |
Whoops, |
Checkout #804 |
We can make initializers generic. |
Related to #557
There might be a better solution, but this pull request does the job, simple and easy.