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

Deprecate Faker::Number.decimal_part and .leading_zero_number #1516

Merged
merged 1 commit into from Jun 22, 2019
Merged

Deprecate Faker::Number.decimal_part and .leading_zero_number #1516

merged 1 commit into from Jun 22, 2019

Conversation

vbrazo
Copy link
Member

@vbrazo vbrazo commented Jan 27, 2019

In version 2.0, Faker::Number methods are going to return integer, so we need to deprecate a few methods that won't make sense soon.

Checklist

  • Deprecate Faker::Number.decimal_part
  • Deprecate Faker::Number.leading_zero_number

This PR is related to #510.

@vbrazo vbrazo self-assigned this Jan 27, 2019
@vbrazo vbrazo changed the title Deprecate Faker::Number.decimal_part and Faker::Number.leading_zero_number Deprecate Faker::Number.decimal_part and leading_zero_number Jan 27, 2019
@vbrazo vbrazo changed the title Deprecate Faker::Number.decimal_part and leading_zero_number Deprecate Faker::Number.decimal_part and .leading_zero_number Jan 27, 2019
@vbrazo vbrazo merged commit 1950a3a into faker-ruby:master Jun 22, 2019
@vbrazo vbrazo deleted the deprecate/leading-number-and-decimal-part branch June 22, 2019 16:17
@tjwallace
Copy link

Faker::Number.decimal uses Faker::Number.decimal_part - is there a replacement?

@vbrazo
Copy link
Member Author

vbrazo commented Jul 5, 2019

Yeah! Faker::Number.decimal should've been deprecated too. Thanks for letting me know!

There is no replacement yet @tjwallace any suggestions?

@vbrazo
Copy link
Member Author

vbrazo commented Jul 5, 2019

@tjwallace the replacement would be converting the strings to decimals.

I'll think about it. Feel free to open a PR in case you come up with a good idea.

If we make it, we don't need to deprecate/remove the methods.

@dadamschi
Copy link

If I think I am following this correctly, Faker:Number.decimal in v2 is not using Faker:Number.decimal_part.
https://github.com/stympy/faker/blob/v2/lib/faker/default/number.rb#L14

@vbrazo
Copy link
Member Author

vbrazo commented Jul 7, 2019

@dadamschi I haven't had time to change it yet, but our final idea was to keep Faker:Number.decimal and Faker:Number.decimal_part and return them as decimal instead of string.

@dadamschi
Copy link

I think that makes sense. Thanks for clearing that up.

And, FWIW, what is returned for decimal_part is called fractional_part. https://en.wikipedia.org/wiki/Fractional_part

@aleksejkuzmin
Copy link

aleksejkuzmin commented Jul 16, 2019

I like the idea to return decimal instead of string, but faker itself uses deprecated methods, thus you have deprecation messages even if you don't use deprecated methods directly. E.g. with faker-1.9.6

[1] 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/hideman/.rvm/gems/ruby-2.5.1/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/hideman/.rvm/gems/ruby-2.5.1/gems/faker-1.9.6/lib/faker/default/number.rb:29.
=> "27863.07"

Doesn't it make sense to change internal implementation too? Otherwise tests output become messy

@vbrazo
Copy link
Member Author

vbrazo commented Jul 16, 2019

@aleksejkuzmin this issue talks about that #1655 and it'll be fixed in v2.

@aleksejkuzmin
Copy link

@vbrazo sorry, didn't spot that issue. thanks for answering!

michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants