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

Enable Explicit Self Swiftlint rule #61

Closed
kgellci opened this issue Feb 24, 2019 · 8 comments
Closed

Enable Explicit Self Swiftlint rule #61

kgellci opened this issue Feb 24, 2019 · 8 comments
Labels
good first issue Good for newcomers

Comments

@kgellci
Copy link
Owner

kgellci commented Feb 24, 2019

https://github.com/realm/SwiftLint/blob/master/Rules.md#explicit-self

Enable explicit self in our swiftlint usage. This will help keep the code across the project consistent and make it more clear in a function if a variable being accessed belongs to the class or is defined somewhere in the function.

Instructions: https://github.com/realm/SwiftLint#configuration

@kgellci kgellci added the good first issue Good for newcomers label Feb 24, 2019
@RJ-Clegg
Copy link
Collaborator

RJ-Clegg commented Feb 24, 2019

I'm not sure I agree this is a useful addition. Generally it just creates more noise in the code. It's a common accepted styling guide to omit it outside of initialisers and closures as the Swift compiler doesn't require it.

The Swift core team struck a good balance I feel with this approach.

I've seen it in other styling guides such as the one from Ray Wenderlich - to quote from the guide:

For conciseness, avoid using self since Swift does not require it 
to access an object's properties or invoke its methods.

Use self only when required by the compiler 
(in @escaping closures, or in initializers to disambiguate properties from arguments). 
In other words, if it compiles without self then omit it.

I prefer this style as it's 99% of the time clear where the definition is.

Perhaps we should start a style guide and host it on the repo where people can collaborate on what should and shouldn't be in it?

@kgellci
Copy link
Owner Author

kgellci commented Feb 24, 2019

I want to link the Swift proposal for requiring explicit self as well as why it was rejected:

I will post on r/swift to hopefully have a broader convo. I am okay with either way as long as we can consistently enforce one or the other.

@Jumhyn
Copy link

Jumhyn commented Feb 24, 2019

Personally, I'm in favor of requiring explicit self, in large part because of code review (as was mentioned in the Reddit thread). Most code review platforms don't or aren't able to do the analysis required to highlight local variables differently than instance variables, and explicit self keeps the reviewer from having to double check whether each usage is an instance variable or a declaration that they somehow missed on an earlier line.

@danielt1263
Copy link

I expect that everybody already knows this but I think it bears saying none-the-less. To require explicit self would be very much against all the most popular and established swift guidelines and most if not all of the ones in existence... Is that something you really want to do?

https://github.com/raywenderlich/swift-style-guide#use-of-self

For conciseness, avoid using self since Swift does not require it to access an object's properties or invoke its methods.

Use self only when required by the compiler (in @escaping closures, or in initializers to disambiguate properties from arguments). In other words, if it compiles without self then omit it.

https://github.com/linkedin/swift-style-guide

3.1.11 Prefer not writing self. unless it is required.

https://github.com/github/swift-style-guide

Only explicitly refer to self when required

When accessing properties or methods on self, leave the reference to self implicit by default:

https://www.appsfoundation.com/post/swift-code-convention-and-guidelines

Self

Avoid using the word self because it is not required in SWIFT for object properties or methods.

https://engineering.vokal.io/iOS/CodingStandards/Swift.md.html#usage-of-self

Usage of self

Except where necessary, avoid using self.. If you have a local variable that conflicts with a property name or are in a context where self is captured, you may need to use self..

@kgellci
Copy link
Owner Author

kgellci commented Feb 25, 2019

@danielt1263 Thanks for the feedback. In general, guidelines are good practice to follow! That being said, they are guides, not rules. Different projects and project intentions may benefit from not following certain guidelines. Even the core Swift team is aware of this and mentioned it as a reason to why they did not accept a proposal to require explicit self:

Individuals or teams that feel that explicit “self.” is beneficial for their own code bases can enforce such a coding convention via tooling with the status quo. If this proposal were accepted, those opposed to the proposal would effectively have no recourse because the language itself would be enforcing “self.”.

There are some great arguments for and against enforcing explicit self usage. The author of the original proposal summed them up pretty well, I suggest you look through it: https://github.com/apple/swift-evolution/blob/master/proposals/0009-require-self-for-accessing-instance-members.md

The two that interest me the most are:

Clarity is more important than brevity. Although Swift code can be compact, it is a non-goal to enable the smallest possible code with the fewest characters. Brevity in Swift code, where it occurs, is a side-effect of the strong type system and features that naturally reduce boilerplate.

  • Less confusing from a learning point of view.
    Since the goal of this project is to help more noob Swift developers learn, they would benefit from the additional clarity.

@danielt1263
Copy link

@kgellci I don't agree that it would be less confusing if this particular project emits compiler errors when self is not used, despite the fact that the Swift Programming Language book says it's not required and every public guideline says not to use it. I think that would be more confusing, not less. I think it would be a PITA to have to constantly explain to new programmers why this project requires self when no other project they are working in does.

All that said, I'm not a maintainer nor do I plan on contributing anytime soon, so my comments probably aren't important.

@kgellci
Copy link
Owner Author

kgellci commented Feb 25, 2019

I think it would be a PITA to have to constantly explain to new programmers why this project requires self when no other project they are working in does.

That is a good point and will probably be the main reason to not enforce. I am also considering enforcing not using explicit self, if that is the decision, but I do not see a clean way to do it. Maybe will open an issue on Swiftlint.

@kgellci
Copy link
Owner Author

kgellci commented Feb 25, 2019

After some debate, closing this issue. Main reason being, it may be confusing to swift noobs who are learning using multiple sources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants