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 number_separator when the number is wrapped in parentheses. #2687

Merged

Conversation

daltonclaybrook
Copy link
Contributor

Resolves: #2683

@SwiftLintBot
Copy link

SwiftLintBot commented Mar 25, 2019

1 Warning
⚠️ This PR may need tests.
12 Messages
📖 Linting Aerial with this PR took 2.01s vs 1.94s on master (3% slower)
📖 Linting Alamofire with this PR took 4.18s vs 4.14s on master (0% slower)
📖 Linting Firefox with this PR took 13.02s vs 13.04s on master (0% faster)
📖 Linting Kickstarter with this PR took 22.04s vs 22.09s on master (0% faster)
📖 Linting Moya with this PR took 2.04s vs 2.04s on master (0% slower)
📖 Linting Nimble with this PR took 1.82s vs 1.85s on master (1% faster)
📖 Linting Quick with this PR took 0.58s vs 0.59s on master (1% faster)
📖 Linting Realm with this PR took 3.59s vs 3.57s on master (0% slower)
📖 Linting SourceKitten with this PR took 1.16s vs 1.17s on master (0% faster)
📖 Linting Sourcery with this PR took 4.33s vs 4.43s on master (2% faster)
📖 Linting Swift with this PR took 27.97s vs 27.63s on master (1% slower)
📖 Linting WordPress with this PR took 23.05s vs 22.79s on master (1% slower)

Generated by 🚫 Danger

Copy link
Collaborator

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fast PR! Looks good overall, I added two comments to make the code more understadable for future contributors. Please answer the questions I asked and add comments explaining the code where it isn't obvious like suggested.

Copy link
Collaborator

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for considering the feedback. This looks good to me now.

@Jeehut
Copy link
Collaborator

Jeehut commented Mar 27, 2019

Ah, one last thing before I merge this: The SwiftLintBot shows that this change makes many projects considerably slower, which makes sense given the fact, that we run the code for every numeral used in code. We should think about ways to make this faster again, but before doing that we should check that those numbers are actually reliable given that there was an Xcode update recently and that could be behind those changes, too.

@marcelofabri Is there a way we can re-run the SwiftLintBot manually on this PR to evaluate performance of changes? Also what is a significant perfmormance change in you opinion that would hold off from merging a bug fix?

@daltonclaybrook daltonclaybrook force-pushed the dc_number_separator_false_positive branch from 1120b8b to 2567900 Compare March 27, 2019 11:55
@daltonclaybrook
Copy link
Contributor Author

@Dschee Looks like the performance warnings may have been a false-positive.

@Jeehut
Copy link
Collaborator

Jeehut commented Mar 27, 2019

Nice, just took a while to update. Merging. 👏

@Jeehut Jeehut merged commit 9d0ac2d into realm:master Mar 27, 2019
@daltonclaybrook daltonclaybrook deleted the dc_number_separator_false_positive branch March 28, 2019 01:39
@marcelofabri
Copy link
Collaborator

@marcelofabri Is there a way we can re-run the SwiftLintBot manually on this PR to evaluate performance of changes? Also what is a significant perfmormance change in you opinion that would hold off from merging a bug fix?

You should be able to retrigger it from Azure Pipelines. However, it's usually better to run locally to be sure and avoid any infrastructure issues (CI is shared, etc).

"let exp = \(sign)1_000_000.000_000e2"
"let exp = \(sign)1_000_000.000_000e2",
"let foo: Double = \(sign)(200)",
"let foo: Double = \(sign)(200 / 447.214)"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a triggering example with parenthesis to document the behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely. I'll make a separate PR.

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.

4 participants