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

[WIP] Missing Access Control rule #808

Closed
wants to merge 2 commits into from
Closed

Conversation

KaiCode2
Copy link

@KaiCode2 KaiCode2 commented Sep 7, 2016

Implemented a new rule to ensure access control is explicitly set for all classes, structs and enums.

Notes

  • Tests are currently not passing because not all types within the project have adopted this rule.
  • The current implementation is to apply a regex to all lines in project and check for hits. Ideally we would use sourcekitten for the implementation however sourcekit does not output wether the type has been explicitly typed (as default is internal).
  • The regex is not iron clad and there are probably edge cases that would set it off incorrectly… So please give review the regex expression as my regex knowledge is pretty minimal.

TODOs

  • update CHANGELOG.md file
  • incorporate this change across the entire project to make tests pass
  • git squash

@jpsim
Copy link
Collaborator

jpsim commented Nov 23, 2016

Thanks for the PR, @KaiCode2! Sorry for the delay in reviewing this, I'm massively behind on SwiftLint notifications 😅

Have you considered using SourceKitten's parse structure to detect classes, structs and enums rather than attempting to detect them via regex?

For example, we leverage the parse structure to apply certain rules only to class/struct/enum declarations for TypeNameRule: https://github.com/realm/SwiftLint/blob/0.13.0/Source/SwiftLintFramework/Rules/TypeNameRule.swift#L48-L53

This leads to a much more reliable rule since you don't accidentally capture say sample code in comments, since we use the Swift compiler's very accurate parser. Trying to parse Swift by using regular expressions is guaranteed to lead to false positives, and generally more complex less maintainable code.

Given this, the missing "TODO" items from your initial comment, and how outdated this PR is, I'll be closing this for now.

However, I hope you'll be able to take this feedback under consideration and submit a PR again soon!

@jpsim jpsim closed this Nov 23, 2016
@KaiCode2
Copy link
Author

KaiCode2 commented Nov 24, 2016

@jpsim I initially tried to use sourcekitten to detect access control, however because sourcekit infers unset access control to internal and doesn't emit wether it was explicitly set, I am unable to hook into sourcekit (and thus sourcekitten) for this PR.

A potential optimization would be to check type declaration line numbers and explicitly apply the regex to those lines (drastically cutting down on the number of lines where this needs to be applied).

If there's any interest in this PR I can update this branch to be mergeable.

@jpsim
Copy link
Collaborator

jpsim commented Nov 24, 2016

A potential optimization would be to check type declaration line numbers and explicitly apply the regex to those lines (drastically cutting down on the number of lines where this needs to be applied).

Taking this hybrid approach isn't just an optimization, it also means that you leverage the parse structure that is available, leading to more accurate results.

I'm certainly interested in seeing this pursued, which is why we're still tracking this as an open issue: #58.

@KaiCode2
Copy link
Author

Fantastic, I'll update to make the Regex work with the parse structure.

Will open a second PR when finished.

Thanks

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

Successfully merging this pull request may close these issues.

2 participants