-
Notifications
You must be signed in to change notification settings - Fork 205
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
Data object encoding #692
base: master
Are you sure you want to change the base?
Data object encoding #692
Conversation
This sets the ivars _before_ calling initialize, which feels wrong. But Data doesn't give us any mechanism for setting the members other than 1) initialize, or 2) drop down into the C API. Since initialize freezes the object, we need to set the ivars before that. I think this is a reasonable compromise—if users need better handling, they can implement their own `encode_with` and `init_with`. But it will lead to unhappy surprises for some users. Alternatively, we could use the C API, similarly to Marshal. Psych _is_ already using the C API for path2class and build_exception. This would be the least surprising behavior for users, I think.
FYI: I haven't put any guards in yet to make this work for older versions of ruby (before |
I'll update this to pass for earlier versions of ruby. But I'm also looking for feedback on the approach. The more I think about it, the more I'm thinking this should use |
1. `defined?(::Data)` is insufficient: there was an earlier `::Data` class before the new one was added. Checking for `Data.define`. 2. arg forwarding with `def foo(...)` wasn't introduced until 2.7.3. 3. implicit hash/kwparam values wasn't added until 3.1.
3110d41
to
3a845fd
Compare
For what it's worth, I've updated the PR to:
|
The first commit adds basic encoding/decoding for ruby 3.2
Data
objects in (what seems to me) the most straightforward way:Visitor::YamlTree
copies all of the members out into a mapping, andVisitor::ToRuby
calls#initialize
with that mapping. This could lead to side effects, if users have overridden#initialize
in weird ways, but that's true ofStruct
and the attr writer methods and many of the types inVisitors::ToRuby
, too.The second commit handles instance variables, and I feel less happy with that implementation. It sets the ivars before calling initialize, which feels wrong. But
Data
doesn't give us any mechanism for setting the members other than 1)initialize
, or 2) drop down into the C API. Since initialize freezes the object, we need to set the ivars before that. I think this is a reasonable compromise—if users need better handling, they can implement their ownencode_with
andinit_with
. But it will lead to unhappy surprises for some users.Alternatively, maybe we could use the C API, similarly to Marshal. Psych is already using the C API for
path2class
andbuild_exception
. This would be the least surprising behavior for users, I think.Edited to add: I added
ToRuby#init_struct(obj, members)
to the C extension, which delegates torb_struct_initialize
.