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

change return value of `duplicable?` with Ruby 2.4+ #27293

Merged
merged 1 commit into from Dec 13, 2016

Conversation

Projects
None yet
8 participants
@y-yagi
Member

y-yagi commented Dec 7, 2016

Summary

NilClass, FalseClass, TrueClass, Symbol and Numeric can dup
with Ruby 2.4+. Therefore, the result of duplicable? should also be adapted to result of dup.

Ref: https://bugs.ruby-lang.org/issues/12979

@rails-bot

This comment has been minimized.

rails-bot commented Dec 7, 2016

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 7, 2016

Are there any types that are not duplicable in Ruby 2.4? Perhaps we should consider deprecating the method, and defining the dup method on earlier versions of ruby

@matthewd

This comment has been minimized.

Member

matthewd commented Dec 7, 2016

As described at the top of the file, this method exists to avoid the expensive rescue.

We should use the dup+rescue pattern at method definition time, instead -- something similar to 440559f.

@sgrif this seems to leave Method as the only thing we know we can't dup. Maybe it's worth a proposal to fix that too.

@y-yagi y-yagi force-pushed the y-yagi:fix_duplicable_with_2_4 branch Dec 7, 2016

@y-yagi

This comment has been minimized.

Member

y-yagi commented Dec 7, 2016

Thanks for reviewing! I updated to use the dup+rescue pattern at method definition time.

Perhaps we should consider deprecating the method, and defining the dup method on earlier versions of ruby

Is this necessary also?

activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,9 @@
* Change return value of `NilClass#duplicable?`, `FalseClass#duplicable?`
`TrueClass#duplicable?`, `Symbol#duplicable?` and `Numeric#duplicable?`

This comment has been minimized.

@kamipo

kamipo Dec 7, 2016

Member

Missing , between FalseClass#duplicable? and TrueClass#duplicable?.

This comment has been minimized.

@y-yagi

y-yagi Dec 7, 2016

Member

Thanks! I fixed.

@y-yagi y-yagi force-pushed the y-yagi:fix_duplicable_with_2_4 branch Dec 7, 2016

@kaspth

This comment has been minimized.

Member

kaspth commented Dec 12, 2016

@y-yagi needs a rebase :)

change return value of `duplicable?` with Ruby 2.4+
`NilClass`, `FalseClass`, `TrueClass`, `Symbol` and `Numeric` can dup
with Ruby 2.4+.

Ref: https://bugs.ruby-lang.org/issues/12979

@y-yagi y-yagi force-pushed the y-yagi:fix_duplicable_with_2_4 branch to 2cb8558 Dec 12, 2016

@y-yagi

This comment has been minimized.

Member

y-yagi commented Dec 12, 2016

I rebased!

@kaspth kaspth merged commit 04dd8b7 into rails:master Dec 13, 2016

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Member

kaspth commented Dec 13, 2016

Thanks!

@kaspth kaspth removed the needs work label Dec 13, 2016

@y-yagi y-yagi deleted the y-yagi:fix_duplicable_with_2_4 branch Dec 13, 2016

matthewd added a commit to matthewd/rails that referenced this pull request Dec 26, 2016

Merge pull request rails#27293 from y-yagi/fix_duplicable_with_2_4
change return value of `duplicable?` with Ruby 2.4+

matthewd added a commit to matthewd/rails that referenced this pull request Dec 27, 2016

Merge pull request rails#27293 from y-yagi/fix_duplicable_with_2_4
change return value of `duplicable?` with Ruby 2.4+
@matthewrudy

This comment has been minimized.

Contributor

matthewrudy commented Dec 31, 2016

I think this isn't as simple as this for symbols.

Somehow some symbols are duplicable, but others not.

# this is with ruby 2.4.0
>> RUBY_VERSION
=> "2.4.0"

# simple symbols work
>> :a.duplicable?
=> true
>> :a.deep_dup
=> :a
>> :a.dup
=> :a

# but some don't
>> :price_per_km.duplicable?
=> true
>> :price_per_km.deep_dup
TypeError: allocator undefined for Symbol
from /Users/matthew/.rvm/gems/ruby-2.4.0@gogovan/bundler/gems/rails-08af4e1d7fd7/activesupport/lib/active_support/core_ext/object/deep_dup.rb:14:in `dup'
>> :price_per_km.dup
TypeError: allocator undefined for Symbol
from (pry):6:in `dup'

# it doesn't seem to be length related
>> :something_else_long.duplicable?
=> true
>> :something_else_long.deep_dup
=> :something_else_long
>> :something_else_long.dup
=> :something_else_long

I can't work out why :price_per_km isn't duplicable
but we use that symbol a lot
so I guess that's part of it.

junaruga added a commit to junaruga/rails that referenced this pull request Jan 12, 2017

Merge pull request rails#27293 from y-yagi/fix_duplicable_with_2_4
change return value of `duplicable?` with Ruby 2.4+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment