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.

Sign up for GitHub

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

Jump to bottom

Fix non accessible declarations under @objcMembers #2501

Merged
merged 3 commits into from
Dec 5, 2018
Merged

Fix non accessible declarations under @objcMembers #2501

merged 3 commits into from
Dec 5, 2018

Conversation

dirtydanee
Copy link
Contributor

Issue reported as comment under #2270 PR.

@dirtydanee dirtydanee force-pushed the dm-fix-redundant-objc-access-control branch from f15287c to e7d3179 Compare December 4, 2018 20:33
@SwiftLintBot
Copy link

SwiftLintBot commented Dec 4, 2018 •
edited
Loading

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 2.98s vs 2.11s on master (41% slower)
📖 Linting Alamofire with this PR took 6.59s vs 4.37s on master (50% slower)
📖 Linting Firefox with this PR took 23.14s vs 15.73s on master (47% slower)
📖 Linting Kickstarter with this PR took 31.41s vs 23.74s on master (32% slower)
📖 Linting Moya with this PR took 3.3s vs 2.32s on master (42% slower)
📖 Linting Nimble with this PR took 2.88s vs 2.11s on master (36% slower)
📖 Linting Quick with this PR took 0.8s vs 0.7s on master (14% slower)
📖 Linting Realm with this PR took 6.08s vs 4.28s on master (42% slower)
📖 Linting SourceKitten with this PR took 1.82s vs 1.59s on master (14% slower)
📖 Linting Sourcery with this PR took 8.12s vs 6.38s on master (27% slower)
📖 Linting Swift with this PR took 37.39s vs 37.24s on master (0% slower)
📖 Linting WordPress with this PR took 26.29s vs 30.43s on master (13% faster)

Generated by 🚫 Danger

@jpsim
Copy link
Collaborator

jpsim commented Dec 4, 2018

Thanks for the fix! I made some small changes and pushed to your branch:

  • Refactored to avoid false optionality. Because a dictionary was used with two guaranteed keys, this required treating their values as optional even though we knew they weren't. Fixed this by using a struct with two named members instead.
  • Moved the changelog entry to the "Bug Fixes" section like we usually do when fixing false positives for rules.
  • Linked the PR where the issue was reported in the changelog.
  • Hard-wrapped other changelog entries in master that were over the 80 character limit.

I have one last question before we can merge this: why did you bump the minimum Swift version to 4.1? Whatever the reason, it'd be good to add a comment explaining why this rule wouldn't be available to older Swift versions.

@dirtydanee
Copy link
Contributor Author

thanks for cleaning the code up, the PR was originally a bit sloppy.

I have one last question before we can merge this: why did you bump the minimum Swift version to 4.1?

The ACL attribute modifiers (.private and .fileprivate) are only available from Swift 4.1 as part of the SwiftDeclarationAttributeKind.

@jpsim
Copy link
Collaborator

jpsim commented Dec 5, 2018

Sounds good, thanks!

@jpsim jpsim merged commit 58a54db into realm:master Dec 5, 2018
@marcelofabri marcelofabri mentioned this pull request Dec 5, 2018
Closed
2 tasks
@marcelofabri marcelofabri mentioned this pull request Jan 3, 2019
Merged
sjavora pushed a commit to sjavora/SwiftLint that referenced this pull request Mar 9, 2019
@dirtydanee @sjavora
Fix non accessible declarations under @objcMembers (realm#2501)
bfd1faa 
* Fix non accessible declarations under @objcMembers

* Refactor to avoid false optionality

* fixup changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@dirtydanee dirtydanee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dirtydanee @SwiftLintBot @jpsim