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

Fix false positives on redundant_objc_attribute rule when using nested types #2540

Merged
merged 4 commits into from
Jan 4, 2019
Merged

Fix false positives on redundant_objc_attribute rule when using nested types #2540

merged 4 commits into from
Jan 4, 2019

Conversation

marcelofabri
Copy link
Collaborator

Fixes #2539

@marcelofabri
Copy link
Collaborator Author

@dirtydanee @JaviSoto could you take a look at this and see if it makes sense? I'm not super familiar with @objcMembers to be 100% sure this is right.

@SwiftLintBot
Copy link

SwiftLintBot commented Jan 3, 2019

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 2.2s vs 2.04s on master (7% slower)
📖 Linting Alamofire with this PR took 5.27s vs 4.33s on master (21% slower)
📖 Linting Firefox with this PR took 18.64s vs 14.94s on master (24% slower)
📖 Linting Kickstarter with this PR took 28.65s vs 22.66s on master (26% slower)
📖 Linting Moya with this PR took 2.73s vs 2.26s on master (20% slower)
📖 Linting Nimble with this PR took 2.79s vs 2.06s on master (35% slower)
📖 Linting Quick with this PR took 0.73s vs 0.66s on master (10% slower)
📖 Linting Realm with this PR took 5.29s vs 4.1s on master (29% slower)
📖 Linting SourceKitten with this PR took 1.54s vs 1.32s on master (16% slower)
📖 Linting Sourcery with this PR took 6.45s vs 5.42s on master (19% slower)
📖 Linting Swift with this PR took 46.19s vs 33.25s on master (38% slower)
📖 Linting WordPress with this PR took 25.46s vs 23.49s on master (8% slower)

Generated by 🚫 Danger

Rules.md Outdated Show resolved Hide resolved
@dirtydanee
Copy link
Contributor

@dirtydanee @JaviSoto could you take a look at this and see if it makes sense? I'm not super familiar with @objcMembers to be 100% sure this is right.

I think the updated implementation is pretty neat, and its a much better approach to only check the parent structure instead of iterating on the whole file on each declaration kind.

@marcelofabri marcelofabri merged commit 9f7aa14 into realm:master Jan 4, 2019
@marcelofabri marcelofabri deleted the bugfix-2539 branch January 4, 2019 05:46
@JaviSoto
Copy link
Contributor

JaviSoto commented Jan 4, 2019

Hey! Sorry it took me so long to reply. Yeah this code looks so much better! It also solves the scenario I posted on #2539:

@objcMembers
class Foo {
  @objc enum Bar { }
}

But if I add it to the "non triggering examples", it's passing now 👍

@jpsim
Copy link
Collaborator

jpsim commented Jan 4, 2019

Nice work!

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.

5 participants