Fixed returns doc for init methods issue-557

[mohpor]

Related to #557
There might be a better solution, but this pull request does the job, simple and easy.

[codecov-io]

Current coverage is 89.39% (diff: 50.00%)

Merging #739 into master will decrease coverage by 0.12%

@@             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          

Powered by Codecov. Last update 8389aa2...c7d0cf0

[freak4pc]

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
}

[mohpor]

@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 plain init(), that's why I didn't go for the hasPrefix, but I see your point and I think a leading space could help!
• I decided against the guard usage instead of if because the guidelines I'm following, prohibits using guards for case testing and this is clearly testing against different scenarios, but, if SwiftLint suggests I should use guard, 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 is init(), don't you agree? And with this logic what would become of a function with the following syntax: func runAndExit() -> Never { /* ... */ } in Swift 3?

[freak4pc]

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.

[mohpor]

Thanks for the guideline suggestion. That's very helpful. Wilco. :)

[jpsim]

This wouldn't work for failable initializers (init?). You should check if the declaration is a SwiftDeclarationKind.FunctionConstructor instead.

[Grubas7]

@jpsim Agree, I tried this solution but kind is never equal to .FunctionConstructor.
For code:

class TestClass: NSObject {

    /**
     Description about TestClass
     - parameter string: a super string
     - returns: a new class
     */
    override init() {
        super.init()
    }
}

returns .FunctionMethodInstance. I don't know why whole dictionary doesn't contain . FunctionConstructor value.

[jpsim]

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 init and init? please?

[mohpor]

@jpsim Thanks for the heads up.
I have updated the checking mechanism to support failable initializers as well, made it a separate function, and moved checking to bail out of checking earlier.

The point here IMHO is that, a - returns: documentation for constructors should be optional. Because a constructor has a very specific job. To force the existence of returns doc makes documentation verbose. That being said, there are cases where you want to explain something about the constructor's job (especially in case of failable ones), that should be possible too, so I guess a returns doc for constructors should be optional, not mandatory, nor prohibited.

P.S.: I tried using regex to match the initializer syntax but I failed miserably!
Here is the regex code I was trying to use:

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 init() or init?() no matter what!

[freak4pc]

For the regex thing, this should work if you're interested in doing it
(.*)init(\\?)?\\((.*)\\)

Regex in Swift is painful though.

Here's a short Playground test:

[freak4pc]

About your whitespace issue,
Xcode -> Preferences -> Text Editing -> Automatically trim trailing whitespace, including whitespace-only lines

;-)

[mohpor]

The regex you've mentioned would match performinit() too. The leading space should be mandatory I think. a better approach would be to start from the beginning and move from there, check if there is any character and if not a whitespace, then check that before init there is a whitespace. That makes for a relatively complex regex.

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. 👍

[mohpor]

The correct regex I think is something like this: ^((.+)?\s+)?init\?*\(.*\) do you find any caveats whit this @freak4pc ?

[freak4pc]

Nothing that I can notice, it looks good here with ^((.+)?\\s+)?init\\?*\\(.*\\)

[mohpor]

Worked like a charm!

[mohpor]

@jpsim do you need anything to add to this one?

[mohpor]

It's been 8 days! Any news?

[Grubas7]

@mohpor Could you make optional - returns: for constructors, update CHANGELOG.md and squash your commits?

[mohpor]

@Grubas7 I have updated the CHANGELOG.md.
I thought my commits would be squashed by the admin who will merge this pull request. Am I wrong?

[Grubas7]

@mohpor Yeap, you're right. I forgot about this option on GitHub :)

[jpsim]

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.

[jpsim]

@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.

[mohpor]

@jpsim Done.

P.S.: My branch has conflicts now. Should I do anything?

[freak4pc]

@mohpor you'll probably have to rebase against master

[mohpor]

@freak4pc Thanks again. You're the man.

[mohpor]

Hmmmm, rebasing made everyone's commits into this one. I guess I need to close this commit, make a clean, one commit PR instead. Do you agree @freak4pc @jpsim ?

[jpsim]

Hmmmm, rebasing made everyone's commits into this one. I guess I need to close this commit, make a clean, one commit PR instead.

You don't need to close this PR, you can rebase (correctly) and force-push to the same branch.

[mohpor]

@jpsim Sorry for the mixup, I have re-rebased and squashed my commits.
It seems like a simple merge but I learned a ton.
I should thank @freak4pc for his advices too, very helpful.

[freak4pc]

Anytime. Thanks for your consistent help on this PR! @mohpor

[jpsim]

🎉 congrats for landing this @mohpor

[Grubas7]

This bug is still valid for implicitly unwrapped optional, eg:
init!()

[jpsim]

huh, forgot it was possible to make initializers like that.

[mohpor]

Whoops,
Will make a PR soon.

[mohpor]

Checkout #804

[norio-nomura]

We can make initializers generic.