hash.c: Add a feature to manipulate ruby2_keywords flag #2818
Conversation
| static VALUE | ||
| rb_hash_s_ruby2_keywords_p(VALUE dummy, VALUE hash) | ||
| { | ||
| return (RHASH(hash)->basic.flags & RHASH_PASS_AS_KEYWORDS) ? Qtrue : Qfalse; |
There was a problem hiding this comment.
Do you get TypeError when passing non-Hash instance as hash? I would guess no, but I didn't actually check. I think TypeError should be raised in this case. Same issue with rb_hash_s_ruby2_keywords_bang below.
There was a problem hiding this comment.
Good catch! I've pushed the fix. Thanks
mame
force-pushed
the
ruby2_keywords-flag-manipulation
branch
from
6753a0d to
c3a626a
Compare
There was a problem hiding this comment.
Daniel DeLorme made a good comment in https://bugs.ruby-lang.org/issues/16486#note-5.
I think there should be no way to change the ruby2_keywords flag inplace.
Instead, I think we should have hash2 = Hash.mark_ruby2_keywords(hash) (or Hash.as_ruby2_keywords) with hash2 flagged.
Having a method Hash.ruby2_keywords would be problematic as it would conflict with Module#ruby2_keywords.
|
If there is a way to selialize and deselialize a flagged hash, we as Rails team doesn't require this utility methods. If this methods are for casual use, I agree that a casual method should not mutate an argument (at least should have an alternative non-mutation version). If there are two version methods (mutation (non-allocation) version and non-mutation (extra allocation) version), I suppose we would use non-allocation version in the activejob case. https://gist.github.com/mame/18cd2dfc7d965d0bad1216f2fdd008ee#file-bouncing_test-patch-L152 |
|
@eregon IMO, the mutating version is enough here because it is not for a casual use; in the ActiveJob case, mutating version is slightly preferable (as @kamipo said), and currently there is no known use case where non-mutating version is preferable. But I'll discuss the behavior (and method name) at the next dev-meeting.
Good point, I didn't notice. Thanks! @kamipo Thank you for your comment.
As far as I understand, ActiveJob has a dedicated serialization mechanism which cannot be controlled by the ruby core. So, all we (ruby-core team) can do is to provide a way to check and add the ruby2_keywords flag, and I'd like to ask you to use the feature to fix ActiveJob serialization code.
Yes, that is why I'm going to introduce mutating version. |
| rb_hash_s_ruby2_keywords_bang(VALUE dummy, VALUE hash) | ||
| { | ||
| Check_Type(hash, T_HASH); | ||
| RHASH(hash)->basic.flags |= RHASH_PASS_AS_KEYWORDS; |
There was a problem hiding this comment.
Should this check if the Hash is frozen?
There was a problem hiding this comment.
Ah, good point. Should we raise an exception?
There was a problem hiding this comment.
I think so, yes. But I would prefer if it was not mutating at all.
The problem is this tiny decision completely prevents other implementations or designs for ruby2_keywords. For instance, if we wanted to try using a KwHash subclass instead of a flag, as suggested in https://bugs.ruby-lang.org/issues/16463#note-20, the sole existence of So I think making it mutating here is needlessly restricting, and I oppose it. Maybe we should not even introduce a new way to flag, as it's so easy to do it manually: ruby2_keywords def flag_kwhash(*args)
args.last
end
kw = flag_kwhash(**h) |
mame
force-pushed
the
ruby2_keywords-flag-manipulation
branch
2 times, most recently
from
4921c8b to
03bed4d
Compare
It was found that a feature to check and add ruby2_keywords flag to an existing Hash is needed when arguments are serialized and deserialized. It is possible to do the same without explicit APIs, but it would be good to provide them as a core feature. rails/rails#38105 (comment) Hash.ruby2_keywords_hash?(hash) checks if hash is flagged or not. Hash.ruby2_keywords_hash(hash) returns a duplicated hash that has a ruby2_keywords flag, [Bug #16486]
mame
force-pushed
the
ruby2_keywords-flag-manipulation
branch
from
03bed4d to
7d6ddcc
Compare
Related rails#38053, rails#38187, rails#38105. This is a reattempt to fix keyword arguments warnings in Active Job. Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be backported to 2.7.1. ruby/ruby#2818 https://bugs.ruby-lang.org/issues/16486 I've emulated that for 2.7.0 and older versions.
In ruby#2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
In #2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
Related rails#38053, rails#38187, rails#38105, rails#38260. This is a reattempt to fix keyword arguments warnings in Active Job. Now Ruby (master) has `Hash.ruby2_keywords_hash{?,}` and that will be backported to 2.7.1. ruby/ruby#2818 https://bugs.ruby-lang.org/issues/16486 I've emulated that for 2.7.0 and older versions.
In #2818, `Hash.ruby2_keywords!` has renamed to `Hash.ruby2_keywords_hash`.
4 participants
It was found that a feature to check and add ruby2_keywords flag to an
existing Hash is needed when arguments are serialized and deserialized.
It is possible to do the same without explicit APIs, but it would be
good to provide them as a core feature.
rails/rails#38105 (comment)
Hash.ruby2_keywords?(hash) checks if hash is flagged or not.
Hash.ruby2_keywords!(hash) flags a given hash.