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

Rule request: explicit access control level #1649

Closed
jdmarshall90 opened this Issue Jul 1, 2017 · 10 comments

Comments

Projects
None yet
6 participants
@jdmarshall90

jdmarshall90 commented Jul 1, 2017

See title

I know there is a rule for top level ACL declarations, but one for all declarations would be nice. This would probably be opt-in, as it would mostly be useful by frameworks and not applications.

@masters3d

This comment has been minimized.

Show comment
Hide comment
@masters3d

masters3d Jul 10, 2017

Contributor

Do you have any examples on how explicit non top level declarations would benefit from this requested rule?
The use case from the top level ACL rule is frameworks.

Contributor

masters3d commented Jul 10, 2017

Do you have any examples on how explicit non top level declarations would benefit from this requested rule?
The use case from the top level ACL rule is frameworks.

@jdmarshall90

This comment has been minimized.

Show comment
Hide comment
@jdmarshall90

jdmarshall90 Jul 13, 2017

I don't have any specific examples; this is just more of a personal preference that I like to follow when developing frameworks. It gives more context to anything that you're looking at within the library.

jdmarshall90 commented Jul 13, 2017

I don't have any specific examples; this is just more of a personal preference that I like to follow when developing frameworks. It gives more context to anything that you're looking at within the library.

@mshibanami

This comment has been minimized.

Show comment
Hide comment
@mshibanami

mshibanami Jul 26, 2017

Contributor

+1 because I'm a fan of this opinion:

Because default access levels vary across languages, and many people program in more than one language. It's easy to become confused, either as the author or as someone reading the code later, thus explicit is nicer to deal with than implicit.

Contributor

mshibanami commented Jul 26, 2017

+1 because I'm a fan of this opinion:

Because default access levels vary across languages, and many people program in more than one language. It's easy to become confused, either as the author or as someone reading the code later, thus explicit is nicer to deal with than implicit.

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Jul 26, 2017

Collaborator

@mshibanami but adding them to the top level declarations should be enough to cover that case

Collaborator

marcelofabri commented Jul 26, 2017

@mshibanami but adding them to the top level declarations should be enough to cover that case

@mshibanami

This comment has been minimized.

Show comment
Hide comment
@mshibanami

mshibanami Jul 26, 2017

Contributor

@marcelofabri

internal class Hello {
    let v = "world"
}

In this case, I think it would be nice if SwiftLint warns me about v doesn't have a explicit access modifier. But it seems that explicit_top_level_acl doesn't work so.

I'm sorry if I misunderstood.

Contributor

mshibanami commented Jul 26, 2017

@marcelofabri

internal class Hello {
    let v = "world"
}

In this case, I think it would be nice if SwiftLint warns me about v doesn't have a explicit access modifier. But it seems that explicit_top_level_acl doesn't work so.

I'm sorry if I misunderstood.

@marcelofabri

This comment has been minimized.

Show comment
Hide comment
@marcelofabri

marcelofabri Jul 26, 2017

Collaborator

That rule won't trigger a violation because it only validates top level declarations (as Hello in your example).

I actually got your point now. Sorry for adding noise to the conversation.

Collaborator

marcelofabri commented Jul 26, 2017

That rule won't trigger a violation because it only validates top level declarations (as Hello in your example).

I actually got your point now. Sorry for adding noise to the conversation.

@joseprl89

This comment has been minimized.

Show comment
Hide comment
@joseprl89

joseprl89 Nov 25, 2017

Contributor

+1 to this. I've seen many Swift codebases that neglect using ACLs in the internals of a class/enum/struct. I'm unsure why this is so pervasive specifically on iOS, but I'd prefer to ensure that my team does not neglect something as crucial as class encapsulation.

My concern on class encapsulation is due to Swift defaulting to internal, and as opposed to Java where that's package protected, internal means your whole target can see that field.

In summary, adding this rule would improve the health of any codebase that opts in, so I'm a bit surprised it's not already baked in.

Contributor

joseprl89 commented Nov 25, 2017

+1 to this. I've seen many Swift codebases that neglect using ACLs in the internals of a class/enum/struct. I'm unsure why this is so pervasive specifically on iOS, but I'd prefer to ensure that my team does not neglect something as crucial as class encapsulation.

My concern on class encapsulation is due to Swift defaulting to internal, and as opposed to Java where that's package protected, internal means your whole target can see that field.

In summary, adding this rule would improve the health of any codebase that opts in, so I'm a bit surprised it's not already baked in.

@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Nov 29, 2017

Collaborator

@joseprl89 since you appear to feel strongly about this, perhaps you're motivated enough to contribute this rule to SwiftLint yourself? I'm happy to provide guidance if you get stuck.

Collaborator

jpsim commented Nov 29, 2017

@joseprl89 since you appear to feel strongly about this, perhaps you're motivated enough to contribute this rule to SwiftLint yourself? I'm happy to provide guidance if you get stuck.

joseprl89 added a commit to joseprl89/SwiftLint that referenced this issue Dec 2, 2017

@joseprl89

This comment has been minimized.

Show comment
Hide comment
@joseprl89

joseprl89 Dec 2, 2017

Contributor

@jpsim I had some time that I could chip into it, please review #1966 and let me know if I missed anything 👍

Contributor

joseprl89 commented Dec 2, 2017

@jpsim I had some time that I could chip into it, please review #1966 and let me know if I missed anything 👍

jpsim added a commit that referenced this issue Dec 22, 2017

Merge pull request #1966 from joseprl89/feature/explicit_acl_rule
Add explicit acl rule to satisfy Issue #1649
@jpsim

This comment has been minimized.

Show comment
Hide comment
@jpsim

jpsim Dec 22, 2017

Collaborator

Done in #1966.

Collaborator

jpsim commented Dec 22, 2017

Done in #1966.

@jpsim jpsim closed this Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment