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

False positive on rule `number_separator` #2637

Closed
Dschee opened this Issue Feb 12, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@Dschee
Copy link
Collaborator

Dschee commented Feb 12, 2019

Having the rule number_separator turned on for large numbers, years should still be possible to be written without those separators. For example, the following code should not trigger the rule:

HolidayCalendar.Day(year: 2019, month: 12, day: 25)

Rationale: Writing year literals with a separator like 2_019 actually makes them less readable and should be made an exception for. Of course, it could also be the number 2_019 as a random case but developers should be allowed to give the number the look of a year if it is a year.

@Dschee Dschee self-assigned this Feb 12, 2019

@Dschee Dschee changed the title False positives on rule `number_separator` False positive on rule `number_separator` Feb 12, 2019

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Feb 12, 2019

I don't think this is a false positive: you can configure the rule to only trigger after 4 digits.

In retrospect, maybe that should be the default, but changing it now would cause a lot of issues.

@Dschee

This comment has been minimized.

Copy link
Collaborator Author

Dschee commented Feb 12, 2019

Maybe I should change the region to 1900-2030, but I think the typical year numbers should be excludable, either by default (which is what I'm suggesting here) or with an option that not only allows to specify the number of digits, but rather an excludable range or even just yearsAllowed as a Bool. Cause I don't wan't 4096 to be possible (4_096 is better here) but I want to be able to use common year numbers nowadays.

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Feb 12, 2019

What do you think about creating a configuration option that allows you to specify a valid range instead?

I don't feel like trying to assume if a number is a year to be a good idea (and neither a good default behavior).

@Dschee

This comment has been minimized.

Copy link
Collaborator Author

Dschee commented Feb 12, 2019

I'm fine with that, let me see ...

@Dschee

This comment has been minimized.

Copy link
Collaborator Author

Dschee commented Feb 12, 2019

How would I call the properties, does YAML support ranges? If not, how to name them, validRangeLowerBound and validRangeUpperBound? Also, what about supporting multiple valid ranges?

@marcelofabri

This comment has been minimized.

Copy link
Collaborator

marcelofabri commented Feb 12, 2019

I don't think YAML supports ranges. We could have something like:

valid_ranges:
  - min: 0
    max: 200
  - min: 5000
    max: 10000
@Dschee

This comment has been minimized.

Copy link
Collaborator Author

Dschee commented Feb 12, 2019

Okay, I've made the change. Should be ready to merge once the CI passes.

@Dschee Dschee closed this in #2642 Feb 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.