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

valid_doc false negative #758

Closed
agordeev opened this issue Aug 17, 2016 · 18 comments
Closed

valid_doc false negative #758

agordeev opened this issue Aug 17, 2016 · 18 comments
Labels
bug Unexpected and reproducible misbehavior.

Comments

@agordeev
Copy link

The function below returns warning: Valid Docs Violation: Documented declarations should be valid. (valid_docs)

/**
 Sets alpha to all subviews of a specific `UIView`, except of given subview.

 - parameter alpha: New alpha value for subviews
 - parameter view: UIView the action should be applied to
 - parameter exceptView: The view, that should not change it's alpha
 - parameter duration: Duration of the animation in seconds
 */
class func setAlpha(alpha alpha: CGFloat, view: UIView, exceptSubview: UIView, duration: NSTimeInterval) {
}

Is there any way to check what exactly causes this warning?

@Yaro812
Copy link

Yaro812 commented Aug 17, 2016

check parameter name for exceptView (it is exceptSubview in your method)
you might want to use VVDocumenter-Xcode plugin that handles documentation templates automatically

@agordeev
Copy link
Author

@Yaro812 Thanks a lot, my bad!

However, this one shows the warning as well:

/**
 Returns messages for given conversation.

 - parameter conversation: A conversation.
 - parameter limit: Defines max count of returned messages.
 - parameter success: Success closure.
 - parameter failure: Failure closure.
*/
func obtainPreviousMessagesForConversation(conversation: Conversation, limit: Int, success: SuccessWithEntitiesCompletionHandler, failure: FailureCompletionHandler) {
}

@freak4pc
Copy link
Contributor

freak4pc commented Aug 17, 2016

Just tested it and not getting a documentation error on it.

14:26:39 freak4pc » swiftlint                                                                                                                                      

Linting Swift files in current working directory
Linting 't.swift' (1/1)
/Users/freak4pc/Work/Tests/lint/t.swift:9: warning: Line Length Violation: Line should be 100 characters or less: currently 166 characters (line_length)
Done linting! Found 1 violation, 0 serious in 1 file.

@Yaro812
Copy link

Yaro812 commented Aug 17, 2016

You are using the completion handler. There might be a slight bug in Swiftlint that shows documentation warning when closure that returns no value is written like (param: Int) -> (). You can rewrite it to be (param: Int) -> Void

@agordeev
Copy link
Author

agordeev commented Aug 18, 2016

@freak4pc Interesting. Can you please try this one:

/**
 Returns messages for given conversation.

 - parameter conversation: A conversation.
 - parameter limit: Defines max count of returned messages.
 - parameter success: Success closure.
 - parameter failure: Failure closure.
*/
func obtainPreviousMessagesForConversation(conversation: String, limit: Int, success: (() -> Void)?, failure: (() -> Void)?) {
}

I'm getting a warning with this.

By the way, my completion handlers look like this:

typealias SuccessWithEntitiesCompletionHandler = ((entities: [T]) -> Void)?
typealias FailureCompletionHandler = ((allowRetry: Bool, errorMessage: String) -> Void)?

If the above piece compiles fine for you, may be I should update my SwiftLint? The current version is 0.10.0.

@freak4pc
Copy link
Contributor

I'm a bit busy atm will check later but you should definitely update. Current is 0.11.1 afaik.

@agordeev
Copy link
Author

@freak4pc Thanks. brew says swiftlint 0.10.0 already installed.

@freak4pc
Copy link
Contributor

@andrew8712 Did you try brew update && brew upgrade swiftlint ?

@agordeev
Copy link
Author

@freak4pc No, sorry. Updated fine now, however, getting the same warning with swiftlint 0.11.1

@agordeev
Copy link
Author

@freak4pc I found the problem. My func's description starts with Returns word, which caused ValidDocsRule.commentReturns(_:) function returns true. However, the func doesn't return any value, that's why SwiftLint is showing a warning: superfluousReturnDocumentation returns true.

Since my func has completion handlers, I believe the documentation may contain "Returns" keyword. So this condition is not the best fit:

    return comment.lowercaseString.containsString("- returns:") ||
    comment.rangeOfString("Returns")?.startIndex == comment.startIndex

Any ideas how to handle a case with completion handlers?

@jpsim
Copy link
Collaborator

jpsim commented Aug 21, 2016

If you want to bypass SwiftLint's semantics for determining whether or not a function returns, disable the rule.

@freak4pc
Copy link
Contributor

freak4pc commented Aug 22, 2016

You description starts with a - returns: ? as an entire phrase ? that seems odd.
Could you share more details on how that looks in your code?

Edit: Oh sorry, just noticed that second part of the condition. @jpsim Is there any reason why this couldn't be omitted? If a method returns and doesn't include a - returns: it should result in a valid_docs rule warning. I'm not sure a description starting with Returns fits as a rule ?

@jpsim
Copy link
Collaborator

jpsim commented Aug 22, 2016

A description starting with Returns is treated the same was as if - returns: had been included.

I don't think blocks/closures that are invoked with parameters count as "returning" in the same sense, and therefore shouldn't use either approach.

@jpsim jpsim added the bug Unexpected and reproducible misbehavior. label Jan 21, 2017
@marcelofabri
Copy link
Collaborator

Closing this since we removed this rule.

@freak4pc
Copy link
Contributor

@marcelofabri Mind linking to discussion on that?
Was the valid docs rule removed altogether ?

@marcelofabri
Copy link
Collaborator

This rule wasn't working at all since Swift 2.3. At some point we disabled it on Swift 2.3+ and on the next release we'll drop support for Swift 2, so the rule was removed.

@freak4pc
Copy link
Contributor

Gotya. Thanks for the clarification 👍
Should we investigate rewriting that rule or the consensus is there's no use for it ATM ?

@marcelofabri
Copy link
Collaborator

The issue was that SourceKit stopped providing the information we need to make it work. Check #728 for more info.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected and reproducible misbehavior.
Projects
None yet
Development

No branches or pull requests

5 participants