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.

Sign up for GitHub

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

Jump to bottom

Problem with BigDecimalWithNumericArgument cop #468

Closed
tycooon opened this issue Sep 16, 2024 · 1 comment · Fixed by #469
Closed

Problem with BigDecimalWithNumericArgument cop #468

tycooon opened this issue Sep 16, 2024 · 1 comment · Fixed by #469
Labels
bug Something isn't working

Comments

@tycooon
Copy link

tycooon commented Sep 16, 2024

I have the following code in my app:

BigDecimal("1.0")

Latest rubocop autocorrects it to the following:

BigDecimal(1.0)

The problem is it does not work and I get error:

     ArgumentError:
       can't omit precision for a Float.

1.66.1 (using Parser 3.3.5.0, rubocop-ast 1.32.3, running on ruby 3.3.5) [arm64-darwin23]
  - rubocop-factory_bot 2.26.1
  - rubocop-performance 1.22.0
  - rubocop-rails 2.26.1
  - rubocop-rake 0.6.0
  - rubocop-rspec 3.0.5
@koic koic added the bug Something isn't working label Sep 17, 2024
koic added a commit to koic/rubocop-performance that referenced this issue Sep 17, 2024
@koic
[Fix rubocop#468] Fix false positives for `Performance/BigDecimalWith…
6bb06b2 
…NumericArgument`

Fixes rubocop#468.

This PR fixes false positives for `Performance/BigDecimalWithNumericArgument`
when using float argument for `BigDecimal`.

In Ruby 3.1 and later, cases where numbers are faster than strings are limited to `Integer`.
For `Float`, strings are still faster:

```console
$ cat example.rb
require 'benchmark/ips'
require 'bigdecimal'
require 'bigdecimal/util'

Benchmark.ips do |x|
  x.report('float string')             { BigDecimal('4.2') }
  x.report('float with prec')          { BigDecimal(4.2, Float::DIG + 1) }
  x.report('to_d string without prec') { '4.2'.to_d }
  x.report('to_d float without prec')  { 4.2.to_d }
  x.report('to_d float with prec')     { 4.2.to_d(Float::DIG + 1) }
  x.compare!
end
```

```console
ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [x86_64-darwin22]
Warming up --------------------------------------
        float string   246.214k i/100ms
     float with prec   173.880k i/100ms
to_d string without prec
                       255.950k i/100ms
to_d float without prec
                       181.033k i/100ms
to_d float with prec   151.091k i/100ms
Calculating -------------------------------------
        float string      2.418M (± 5.5%) i/s -     12.064M in   5.004969s
      float with prec      1.685M (± 4.0%) i/s -      8.520M in   5.064059s
to_d string without prec
                          2.460M (± 4.2%) i/s -     12.286M in   5.002392s
to_d float without prec
                          1.781M (± 6.5%) i/s -      8.871M in   5.007829s
to_d float with prec      1.584M (± 5.7%) i/s -      8.008M in   5.072184s

Comparison:
to_d string without prec:  2460462.7 i/s
        float string:  2418003.6 i/s - same-ish: difference falls within error
to_d float without prec:  1781070.6 i/s - 1.38x  slower
     float with prec:  1685372.9 i/s - 1.46x  slower
to_d float with prec:  1584419.5 i/s - 1.55x  slower
```
@koic koic mentioned this issue Sep 17, 2024
Merged
8 tasks
@koic koic closed this as completed in #469 Sep 17, 2024
koic added a commit that referenced this issue Sep 17, 2024
@koic
Merge pull request #469 from koic/fix_false_positives_for_performance…
100cdfe 
…_big_decimal_with_numeric_argument_cop

[Fix #468] Fix false positives for `Performance/BigDecimalWithNumericArgument`
@koic
Copy link
Member

koic commented Sep 17, 2024

This issue has been fixed in RuboCop Performance 1.22.1. Thank you for the feedback!
https://github.com/rubocop/rubocop-performance/releases/tag/v1.22.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@koic @tycooon