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

Deprecation warnings coming from within the library #1655

Closed
nathanhinchey opened this issue Jul 10, 2019 · 12 comments · Fixed by #1651
Closed

Deprecation warnings coming from within the library #1655

nathanhinchey opened this issue Jul 10, 2019 · 12 comments · Fixed by #1651

Comments

@nathanhinchey
Copy link

nathanhinchey commented Jul 10, 2019

Describe the bug

In version 1.9.6 calling Numer::decimal results in deprecation warnings, because Number::decimal uses Number::decimal_part.

Additionally, there is a bug in the deprecation warning where it is suggesting an empty string as a replacement. (Same steps to reproduce)

NOTE: Faker::Number.decimal_part is deprecated; use  instead. It will be removed on or after 2019-06-01.
Faker::Number.decimal_part called from /usr/local/bundle/gems/faker-1.9.6/lib/faker/default/number.rb:34.
NOTE: Faker::Number.leading_zero_number is deprecated; use  instead. It will be removed on or after 2019-06-01.
Faker::Number.leading_zero_number called from /usr/local/bundle/gems/faker-1.9.6/lib/faker/default/number.rb:29.

To Reproduce

Use Number::decimal anywhere. I found it using rake test, but the easiest repro is with an interactive ruby terminal, e.g. in pry:

[1] pry(main)> require 'faker'
=> true
[2] pry(main)> Faker::Number.decimal
NOTE: Faker::Number.decimal_part is deprecated; use  instead. It will be removed on or after 2019-06-01.
Faker::Number.decimal_part called from /Users/nathanhinchey/.rvm/gems/ruby-2.6.2/gems/faker-1.9.6/lib/faker/default/number.rb:34.
NOTE: Faker::Number.leading_zero_number is deprecated; use  instead. It will be removed on or after 2019-06-01.
Faker::Number.leading_zero_number called from /Users/nathanhinchey/.rvm/gems/ruby-2.6.2/gems/faker-1.9.6/lib/faker/default/number.rb:29.
=> "78132.07"

Expected behavior

There should not be a deprecation warning for code that is not deprecated. Non-deprecated code should not call deprecated code.

Additionally, the deprecation warning should suggest an alternative instead of an empty string.

@nathanhinchey
Copy link
Author

nathanhinchey commented Jul 10, 2019

Possibly related to #1610 "Faker Deprecation Warnings showing up in Rails Console" ?

@vbrazo
Copy link
Member

vbrazo commented Jul 10, 2019

This issue will be fixed in v2. Thanks for reporting.

@rylanb
Copy link

rylanb commented Jul 10, 2019

This is a little bit frustrating to see happen. I went searching for where I was possibly using Faker incorrectly, but it seems like you are warning yourselves? I can't see some actual CI output currently because I have 100+ lines of these deprecations.

Can this be turned off through a flag? When is v2 releasing if not?

Thanks for our work on the library! Use it all the time.

@vbrazo
Copy link
Member

vbrazo commented Jul 10, 2019

@rylanb sorry about that! We'll remove all the deprecation warnings in v2.

I can commit myself to get the pending things done this weekend and publish v2.

@rylanb
Copy link

rylanb commented Jul 10, 2019

@vbrazo oh no worries. In the grand scheme of things, not a big deal. I was more concerned that it would be months before a v2 might come out. If its a couple weeks, not a big deal.

Thanks for responding!

@connorshea
Copy link
Member

Is the solution for now just to wait, or is there a change that should be made if you're using the decimal method?

@rylanb
Copy link

rylanb commented Jul 23, 2019

@connorshea probably the easiest solution for now is to jump back before the deprecation was added (~> 1.9.4?). Have to wait for v2 to come out, I think?

@connorshea
Copy link
Member

connorshea commented Jul 23, 2019

For now I'm just going to hack around it since I only have one call to the decimal method. It isn't pretty, but it does seem to work:

# Before
Faker::Number.decimal(3, 1)
# After
"#{rand(999)}.#{rand(10)}".to_f

Hopefully that's useful to someone while we wait :P

@vbrazo
Copy link
Member

vbrazo commented Jul 24, 2019

@connorshea it's going to be fixed in this PR #1651. The change is here.

@vbrazo
Copy link
Member

vbrazo commented Jul 24, 2019

Please feel free to review this PR. I need some eyes on it @rylanb @connorshea @nathanhinchey

@rylanb
Copy link

rylanb commented Jul 26, 2019

@vbrazo I tried to review it but the context is beyond me. I think @nathanhinchey got you covered though!

@vbrazo
Copy link
Member

vbrazo commented Jul 26, 2019

@rylanb yeah! He did a good job. I still need to read that PR again and apply any necessary changes. I think we're getting pretty close.

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 a pull request may close this issue.

4 participants