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

Large number 1_000_000_00 show error #948

Closed
Macorreag opened this issue Feb 2, 2022 · 5 comments
Closed

Large number 1_000_000_00 show error #948

Macorreag opened this issue Feb 2, 2022 · 5 comments

Comments

@Macorreag
Copy link

Macorreag commented Feb 2, 2022

Environment

  • Credo version (mix credo -v): 1.5.6-ref.v1.0.0691faf+uncommittedchanges
  • Erlang/Elixir version (elixir -v): Elixir 1.10.4 (compiled with Erlang/OTP 21)
  • Operating system: Free BSD

What were you trying to do?

I'm using credo in a Phoenix project where the Money structure is used and it contains a long number that generates this error.

Money.to_string(Money.new(1_000_000_00, :COP), fractional_unit: false)  # "$1'000.000.00"

Error Generate By credo type Code rediability.

Expected outcome

I would not expect a creedal warning for this case, as the number is well written.

Actual outcome

Code rediability: Large numbers shold be written with underscores

@nathanbegbie
Copy link

I have run in to the same issue - I think the specific issue is the trailing two digits for numbers that have more than 4 digits.

for example:

75_00 gives no credo warning.
750_00 gives Large numbers should be written with underscores: 75_000

using:
:credo, "~> 1.6"
Elixir 1.12.2 (compiled with Erlang/OTP 24)
OS: macOS

rrrene added a commit that referenced this issue Feb 6, 2022
@rrrene
Copy link
Owner

rrrene commented Feb 6, 2022

This is correct and currently by design, but I can see how we would want to allow trailing digits for things like monetary use cases.

I added a check parameter :trailing_digits, which can be used to define how many trailing digits are allowed:

# exactly 2 digits
{Credo.Check.Readability.LargeNumbers, trailing_digits: 2},
# exactly 2 or 4 digits
{Credo.Check.Readability.LargeNumbers, trailing_digits: [2, 4]},
# 2, 3 or 4  digits
{Credo.Check.Readability.LargeNumbers, trailing_digits: 2..4},

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

@nathanbegbie
Copy link

Can confirm that this resolved my issue. Thank you so much for the quick response! 🙌

@Macorreag
Copy link
Author

Sorry for the delay rrrene, I confirm that this solves my problem perfectly 👍 . Thank you very much for the prompt reply 💨
I also tried the command

mix credo.gen.config 

Which includes this in the config file, which is perfect for setting up the credo revision.

@rrrene
Copy link
Owner

rrrene commented Feb 8, 2022

@nathanbegbie @Macorreag This is now live in v1.6.3. Thx again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants