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 #3981] Allow NumericLiterals to be less strict #4045

Merged
merged 1 commit into from
Feb 21, 2017

Conversation

iGEL
Copy link
Contributor

@iGEL iGEL commented Feb 17, 2017

This introduces a new configuration option for Style/NumericLiteral called AllowLessPlacesAtEnd, which allows numbers which don't have 3 places in the last group separated with underscores. This is useful when specifying monetary values in cents (e.g. $123,456.78 as Money.new(123_456_78) instead of Money.new(12_345_678)).

I also updated the message to make clearer, that the underscores should be used as a decimal mark.

This was the behavior of RuboCop until the 0.47.0 release, see #3749 for the change and #3981 for the issue discussing that change.

Questions:

  • Should it be the default? As it was the behavior until recently, I'd say yes.
  • Do I need to update the documentation for the new option and how do I do it? I ran the rake task, but nothing changed.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@@ -12,8 +12,8 @@ class NumericLiterals < Cop
include ConfigurableMax
include IntegerNode

MSG = 'Separate every 3 digits in the integer portion of a number ' \
'with underscores(_).'.freeze
MSG = 'Use underscores(_) as decimal mark, separate every 3 digits '\
Copy link
Contributor

Choose a reason for hiding this comment

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

… as decimal mark, separate …

"… as decimal mark and separate …"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed locally, will push it later with the rest. 👍

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 19, 2017

Should it be the default? As it was the behavior until recently, I'd say yes.

Probably yes.

Do I need to update the documentation for the new option and how do I do it? I ran the rake task, but nothing changed.

That's odd - running the rake task is all that needs to be done. Btw, you should also update the cop's class description with examples illustrating the usage of this option.

I don't like the name of this option, as it's a bit confusing IMO. Maybe it should be "AllowMonetaryFormat" or something like this as this is clearly the only usecase that warrants special handling.

@Drenmi
Copy link
Collaborator

Drenmi commented Feb 20, 2017

Maybe it should be "AllowMonetaryFormat" or something like this as this is clearly the only usecase that warrants special handling.

+1 🙂

@iGEL
Copy link
Contributor Author

iGEL commented Feb 20, 2017

Btw, you should also update the cop's class description with examples illustrating the usage of this option.

That's probably what I've missed.

I don't like the name of this option, as it's a bit confusing IMO. Maybe it should be "AllowMonetaryFormat" or something like this as this is clearly the only usecase that warrants special handling.

Fine with me, but if we make it the default, is should maybe be inversed, for example something like DisallowMonetaryFormat or maybe just Strict. WDYT?

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 20, 2017

Fine with me, but if we make it the default, is should maybe be inversed, for example something like DisallowMonetaryFormat or maybe just Strict. WDYT?

I'm fine either way.

This introduces a new configuration option for `Style/NumericLiteral`
called `Strict`, which requires numbers to have a group of 3 digits at
end. The cop was changed in 0.47.0 for this behavior, but it's a common
practise to specify monetary amounts in cents and then it's useful to
group the cents separately (e.g. $123,456.78 as `Money.new(123_456_78)`
instead of `Money.new(12_345_678)`). So this commit makes the behavior
configurable and reverts the default to the behavior before the 0.47.0
release.
@iGEL
Copy link
Contributor Author

iGEL commented Feb 21, 2017

@bbatsov Thanks, updated.

@bbatsov bbatsov merged commit 7796c09 into rubocop:master Feb 21, 2017
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 21, 2017

👍

@iGEL iGEL deleted the number_literals branch February 22, 2017 14:03
onk added a commit to onk/rubocop that referenced this pull request Mar 27, 2017
`Strict` parameter was added in rubocop#4045, but `default.yml` has not changed.

I saw the following warnings if set `Strict` to my `.rubocop.yml`.

```
Warning: unrecognized parameter Style/NumericLiterals:Strict found in /Users/onaka/src/github.com/onk/onkcop/.rubocop.yml
```
bbatsov pushed a commit that referenced this pull request Mar 27, 2017
`Strict` parameter was added in #4045, but `default.yml` has not changed.

I saw the following warnings if set `Strict` to my `.rubocop.yml`.

```
Warning: unrecognized parameter Style/NumericLiterals:Strict found in /Users/onaka/src/github.com/onk/onkcop/.rubocop.yml
```
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