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

weak_delegate gets triggered on Protocols that don't inherit from class #972

Closed
bsrz opened this issue Dec 12, 2016 · 5 comments
Closed
Labels
enhancement Ideas for improvements of existing features and rules.

Comments

@bsrz
Copy link

bsrz commented Dec 12, 2016

protocol MyClassProtocol: class {}
var delegate: MyClassProtocol? // valid warning

protocol MyProtocol {}
var delegate: MyProtocol? // invalid warning

Not sure how this can be enforced properly, but this rule is not really usable properly unless this is can be possible.

@marcelofabri marcelofabri added the enhancement Ideas for improvements of existing features and rules. label Dec 13, 2016
@marcelofabri
Copy link
Collaborator

Yeah, we can't do that without changing how SwiftLint works (at least I don't see how). See #836 (comment) for more info.

Just curious: do you find yourself using this a lot? I always use : class when creating delegate/datasource protocols just to be sure to make them weak and avoid retain cycles.

@bsrz
Copy link
Author

bsrz commented Dec 14, 2016

We don't have that many.
Arguably, we shouldn't at all.
Perhaps it makes sense to keep it as is so that it acts as a trigger to think about it and if it is truly required, then a // swiftlint:disable:this weak_delegate should do the trick.

@bsrz bsrz closed this as completed Dec 14, 2016
@AliSoftware
Copy link
Contributor

Actually when I wrote the rule I wondered if I should consider the case, but in fact I also wanted for the rule to enforce protocols used as delegates to always be class protocols, because that's how it should always be.

Enforcing delegates to be weak has the (good, imho) consequence of enforcing protocols to be class protocols because non-class protocols can't be marked as weak, so the aim of the rule is actually to kill two birds with one stone, enforcing both class protocols and weak.

Maybe we should just amend the warning message to make it more clear and not only warn that delegates should be marked as weak but should also be class-bound protocols?

@marcelofabri
Copy link
Collaborator

marcelofabri commented Dec 23, 2016

Maybe we could make it another explicit rule that checks for protocol declarations that end with Delegate and aren't marked as : class?

@AliSoftware
Copy link
Contributor

Ha, could be a solution indeed 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Ideas for improvements of existing features and rules.
Projects
None yet
Development

No branches or pull requests

3 participants