Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix identical operands rule #2549

Closed
wants to merge 6 commits into from
Closed

Fix identical operands rule #2549

wants to merge 6 commits into from

Conversation

xavierLowmiller
Copy link
Contributor

(Fixes #2467)

This PR changes the regex to terminate with a $ instead of \b, since \b does not account for the false positives in #2467.

Characters like < and . are valid word boundaries.

@SwiftLintBot
Copy link

SwiftLintBot commented Jan 10, 2019

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 2.43s vs 2.45s on master (0% faster)
📖 Linting Alamofire with this PR took 5.81s vs 5.44s on master (6% slower)
📖 Linting Firefox with this PR took 20.19s vs 18.53s on master (8% slower)
📖 Linting Kickstarter with this PR took 36.22s vs 29.34s on master (23% slower)
📖 Linting Moya with this PR took 3.62s vs 2.45s on master (47% slower)
📖 Linting Nimble with this PR took 3.87s vs 2.43s on master (59% slower)
📖 Linting Quick with this PR took 1.0s vs 0.74s on master (35% slower)
📖 Linting Realm with this PR took 6.45s vs 4.33s on master (48% slower)
📖 Linting SourceKitten with this PR took 1.85s vs 1.44s on master (28% slower)
📖 Linting Sourcery with this PR took 7.86s vs 6.96s on master (12% slower)
📖 Linting Swift with this PR took 48.07s vs 34.92s on master (37% slower)
📖 Linting WordPress with this PR took 29.74s vs 24.22s on master (22% slower)

Generated by 🚫 Danger

@marcelofabri
Copy link
Collaborator

@xavierLowmiller thanks for doing this, but it's not the right fix. Look at the oss-check results. Some of the false positives fixed are valid violations, like XCTAssertTrue(s1 == s1).

@xavierLowmiller
Copy link
Contributor Author

Ok, I’ll give it another shot 👍

Sent with GitHawk

@jpsim
Copy link
Collaborator

jpsim commented Jan 10, 2019

Thanks for the PR! Some of the OSSCheck performance numbers look concerning. Could you run ./script/oss-check --iterations 5 locally to see if there's indeed such a large performance hit with this change?

@xavierLowmiller
Copy link
Contributor Author

I agree that the numbers look pretty bad.

I could not reproduce them locally, however:

Detailed Output
Cloning Aerial
Cloning Alamofire
Cloning Firefox
Cloning Kickstarter
Cloning Moya
Cloning Nimble
Cloning Quick
Cloning Realm
Cloning SourceKitten
Cloning Sourcery
Cloning Swift
Cloning WordPress
Building branch
Linting 5 iterations of Aerial with branch: 1..2..3..4..5
Linting 5 iterations of Alamofire with branch: 1..2..3..4..5
Linting 5 iterations of Firefox with branch: 1..2..3..4..5
Linting 5 iterations of Kickstarter with branch: 1..2..3..4..5
Linting 5 iterations of Moya with branch: 1..2..3..4..5
Linting 5 iterations of Nimble with branch: 1..2..3..4..5
Linting 5 iterations of Quick with branch: 1..2..3..4..5
Linting 5 iterations of Realm with branch: 1..2..3..4..5
Linting 5 iterations of SourceKitten with branch: 1..2..3..4..5
Linting 5 iterations of Sourcery with branch: 1..2..3..4..5
Linting 5 iterations of Swift with branch: 1..2..3..4..5
Linting 5 iterations of WordPress with branch: 1..2..3..4..5
Building master
HEAD is now at e1023897 Add empty changelog section
Linting 5 iterations of Aerial with master: 1..2..3..4..5
Linting 5 iterations of Alamofire with master: 1..2..3..4..5
Linting 5 iterations of Firefox with master: 1..2..3..4..5
Linting 5 iterations of Kickstarter with master: 1..2..3..4..5
Linting 5 iterations of Moya with master: 1..2..3..4..5
Linting 5 iterations of Nimble with master: 1..2..3..4..5
Linting 5 iterations of Quick with master: 1..2..3..4..5
Linting 5 iterations of Realm with master: 1..2..3..4..5
Linting 5 iterations of SourceKitten with master: 1..2..3..4..5
Linting 5 iterations of Sourcery with master: 1..2..3..4..5
Linting 5 iterations of Swift with master: 1..2..3..4..5
Linting 5 iterations of WordPress with master: 1..2..3..4..5
Message: Linting Aerial with this PR took 1.82s vs 1.85s on master (1% faster)
Message: Linting Alamofire with this PR took 2.88s vs 2.98s on master (3% faster)
Message: Linting Firefox with this PR took 11.34s vs 10.97s on master (3% slower)
Message: Linting Kickstarter with this PR took 16.88s vs 15.89s on master (6% slower)
Message: Linting Moya with this PR took 1.72s vs 1.7s on master (1% slower)
Message: Linting Nimble with this PR took 1.33s vs 1.31s on master (1% slower)
Message: Linting Quick with this PR took 0.43s vs 0.42s on master (2% slower)
Message: Linting Realm with this PR took 2.59s vs 2.71s on master (4% faster)
Message: Linting SourceKitten with this PR took 0.93s vs 0.96s on master (3% faster)
Message: Linting Sourcery with this PR took 3.84s vs 3.9s on master (1% faster)
Message: Linting Swift with this PR took 25.9s vs 25.87s on master (0% slower)
Message: Linting WordPress with this PR took 16.27s vs 16.3s on master (0% faster)

@jpsim
Copy link
Collaborator

jpsim commented Jan 16, 2019

Thanks for running that locally @xavierLowmiller! CI runs on VMs and performance numbers can't really be trusted. These used to be more reliable but have gotten flakier over time. I'm planning on evaluating Azure Pipelines to see if that performs any better in the near future.

@jpsim
Copy link
Collaborator

jpsim commented Jan 16, 2019

Merged to master. Thanks @xavierLowmiller!

@jpsim jpsim closed this Jan 16, 2019
@xavierLowmiller xavierLowmiller deleted the fix-identical_operands branch February 14, 2019 15:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants