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

UnderscoresInNumericLiterals: Allow numbers with non standard groupings #4280

Merged
merged 3 commits into from
Dec 26, 2021

Conversation

marschwar
Copy link
Contributor

fixes #4276

I am somewhat unsure how to handle this. In my opinion this rule should have never reported a violation for groups other than three but changing this now will is a breaking change.

@marschwar marschwar changed the title UnderscoresInNumericLiterals: Allow numbers without groups of 3 UnderscoresInNumericLiterals: Allow numbers with non standard groupings Nov 16, 2021
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #4280 (9cc086a) into main (c6f796a) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 9cc086a differs from pull request most recent head c8c9f65. Consider uploading reports for the commit c8c9f65 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4280      +/-   ##
============================================
- Coverage     84.27%   84.24%   -0.03%     
- Complexity     3249     3263      +14     
============================================
  Files           471      472       +1     
  Lines         10261    10315      +54     
  Branches       1805     1821      +16     
============================================
+ Hits           8647     8690      +43     
- Misses          664      666       +2     
- Partials        950      959       +9     
Impacted Files Coverage Δ
...detekt/rules/style/UnderscoresInNumericLiterals.kt 87.23% <100.00%> (+4.30%) ⬆️
...ch/detekt/core/suppressors/AnnotationSuppressor.kt 80.76% <0.00%> (-9.24%) ⬇️
...osch/detekt/rules/coroutines/CoroutinesProvider.kt 100.00% <0.00%> (ø)
.../arturbosch/detekt/core/suppressors/Suppressors.kt
...b/arturbosch/detekt/core/suppressors/Suppressor.kt 69.23% <0.00%> (ø)
...rbosch/detekt/rules/coroutines/InjectDispatcher.kt 81.81% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6f796a...c8c9f65. Read the comment docs.

@BraisGabin
Copy link
Member

What do you think about adding a flag forceGroupsOfThree (we need a better name) that forces the old behaviour? Disabled the rule will allow any type of _. And I think that it should be enabled by default.

@scottkennedy
Copy link
Contributor

I like that, as it keeps the default behavior the same.

@marschwar
Copy link
Contributor Author

I don't love it but for backward compatibility this is most likely our best choice.

@marschwar marschwar force-pushed the bugfix/UnderscoresInNumericLiterals branch from 9cc086a to a286526 Compare November 17, 2021 21:23
companion object {
private val UNDERSCORE_NUMBER_REGEX = Regex("[0-9]{1,3}(_[0-9]{3})*")
private val HAS_ONLY_STANDARD_GROUPING = """\d{1,3}(?:_\d{3})*""".toRegex()
Copy link
Member

Choose a reason for hiding this comment

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

What does the elvis operator here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only marks a non-capture group. I could remove it, if it is confusing.

If you do not need the group to capture its match, you can optimize this regular expression into Set(?:Value)?. The question mark and the colon after the opening parenthesis are the syntax that creates a non-capturing group.

https://www.regular-expressions.info/brackets.html

Copy link
Member

Choose a reason for hiding this comment

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

I think that keeping the regex as simple as possible is better (if it doesn't affects the performance). But to be honest my comment was pure curiosity.

…/rules/style/UnderscoresInNumericLiterals.kt

Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@BraisGabin BraisGabin added this to the 1.20.0 milestone Dec 26, 2021
@BraisGabin BraisGabin merged commit f925506 into detekt:main Dec 26, 2021
@cortinico cortinico added the rules label Jan 3, 2022
@marschwar marschwar deleted the bugfix/UnderscoresInNumericLiterals branch February 1, 2022 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnderscoresInNumericLiterals can't handle non-standard groupings
4 participants