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 custom BigDecimal serialization #13911

Merged
merged 2 commits into from Feb 1, 2014

Conversation

Projects
None yet
3 participants
@davidcelis
Contributor

davidcelis commented Feb 1, 2014

Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
# => "--- !ruby/object:BigDecimal 18:0.1337E2\n...\n"
YAML.load(yaml).class
# => BigDecimal

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
# => "--- 13.37\n...\n"
YAML.load(yaml).class
# => Float

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
require 'active_support/core_ext/big_decimal/conversions'. I would
agree. So, remove active_support/core_ext/big_decimal so that this
extension is no longer required along with active_support/all.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis me@davidcel.is

@davidcelis

This comment has been minimized.

Show comment
Hide comment
@davidcelis

davidcelis Feb 1, 2014

Contributor

I haven't included tests in this PR because I'm not sure how you guys typically approach tests for a mere deprecation. Just let me know if there's something on that front I should do.

Contributor

davidcelis commented Feb 1, 2014

I haven't included tests in this PR because I'm not sure how you guys typically approach tests for a mere deprecation. Just let me know if there's something on that front I should do.

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Feb 1, 2014

Member

require 'active_support/core_ext/big_decimal/conversions' seems like a lot of work, I will see if we can get this merged in upstream BigDecimal. It should be possible given that yaml is supported by stdlib.

Member

zzak commented Feb 1, 2014

require 'active_support/core_ext/big_decimal/conversions' seems like a lot of work, I will see if we can get this merged in upstream BigDecimal. It should be possible given that yaml is supported by stdlib.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 1, 2014

Member

This broke the whole test suite. Can you take a look?

Also I think we should only deprecate the yaml conversion. Right now we are removing more than we need to.

Member

rafaelfranca commented Feb 1, 2014

This broke the whole test suite. Can you take a look?

Also I think we should only deprecate the yaml conversion. Right now we are removing more than we need to.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 1, 2014

Member

BTW, thanks @davidcelis for working on this

Member

rafaelfranca commented Feb 1, 2014

BTW, thanks @davidcelis for working on this

@davidcelis

This comment has been minimized.

Show comment
Hide comment
@davidcelis

davidcelis Feb 1, 2014

Contributor

I had removed the whole file due to the discussion in the linked Issue, but I can remove only the YAML serialization if that's what people decide is a better idea.

Contributor

davidcelis commented Feb 1, 2014

I had removed the whole file due to the discussion in the linked Issue, but I can remove only the YAML serialization if that's what people decide is a better idea.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 1, 2014

Member

We still need the custom to_s implementation so we can't remove it:

lib/active_record/connection_adapters/abstract/quoting.rb
32:        when BigDecimal then value.to_s('F')
70:        when BigDecimal then value.to_s('F')

So I think we should split that file in 2, deprecate the yaml serialization and leave the custom to_s implementation.

Member

rafaelfranca commented Feb 1, 2014

We still need the custom to_s implementation so we can't remove it:

lib/active_record/connection_adapters/abstract/quoting.rb
32:        when BigDecimal then value.to_s('F')
70:        when BigDecimal then value.to_s('F')

So I think we should split that file in 2, deprecate the yaml serialization and leave the custom to_s implementation.

@davidcelis

This comment has been minimized.

Show comment
Hide comment
@davidcelis

davidcelis Feb 1, 2014

Contributor

Gotcha. I'm assuming I should leave the to_d method there as well?

Contributor

davidcelis commented Feb 1, 2014

Gotcha. I'm assuming I should leave the to_d method there as well?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Feb 1, 2014

Member

We can remove the to_d since it was only to Ruby 1.8 compatibility

Member

rafaelfranca commented Feb 1, 2014

We can remove the to_d since it was only to Ruby 1.8 compatibility

@davidcelis

This comment has been minimized.

Show comment
Hide comment
@davidcelis

davidcelis Feb 1, 2014

Contributor

Great. I'll add a separate commit for that; I can squash it later if you want.

Contributor

davidcelis commented Feb 1, 2014

Great. I'll add a separate commit for that; I can squash it later if you want.

@rafaelfranca

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/big_decimal/conversions.rb

davidcelis added some commits Feb 1, 2014

Don't require BigDecimal serialization extension
Rails currently provides an extension to BigDecimal that redefines how
it is serialized to YAML. However, as noted in #12467, this does not
work as expected. When ActiveSupport is required, BigDecimal YAML
serialization does not maintain the object type. It instead ends up
serializing the number represented by the BigDecimal itself which, when
loaded by YAML later, becomes a Float:

```ruby
require 'yaml'
require 'bigdecimal'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class

require 'active_support/all'

yaml = BigDecimal('13.37').to_yaml
YAML.load(yaml).class
```

@tenderlove posits that we should deprecate the custom BigDecimal
serialization and let Ruby handle it. For the time being, users who
require this serialization for backwards compatibility can manually
`require 'active_support/core_ext/big_decimal/yaml_conversions'`.

This will close #12467 and deprecate the custom BigDecimal#to_yaml.

Signed-off-by: David Celis <me@davidcel.is>
Remove BigDecimal#to_d
This was backported for Ruby 1.8 support and is no longer needed.

Signed-off-by: David Celis <me@davidcel.is>

rafaelfranca added a commit that referenced this pull request Feb 1, 2014

Merge pull request #13911 from davidcelis/remove-bigdecimal-serializa…
…tion

Deprecate custom BigDecimal serialization

Conflicts:
	activesupport/CHANGELOG.md

@rafaelfranca rafaelfranca merged commit c87b27e into rails:master Feb 1, 2014

@davidcelis davidcelis deleted the davidcelis:remove-bigdecimal-serialization branch Feb 2, 2014

@davidcelis

This comment has been minimized.

Show comment
Hide comment
@davidcelis

davidcelis Feb 2, 2014

Contributor

Thanks!

Contributor

davidcelis commented Feb 2, 2014

Thanks!

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