-
Notifications
You must be signed in to change notification settings - Fork 204
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
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 |
3110d41
to
3a845fd
Compare
For what it's worth, I've updated the PR to:
|
b6fcb35
to
2292f0b
Compare
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.
2292f0b
to
325b073
Compare
Sorry. I thought I had it passing before, but I must have made a mistake in my earlier tests with ruby 2.5 (perhaps I had somehow disabled warnings locally)? I've changed the guards from |
Any thoughts about this approach? (Is it too close to the 3.4.0 release to hope for getting support for |
This adds basic encoding/decoding for ruby 3.2
Data
objects, using!ruby/data:ClassName
(a mapping of member names to member values) and!ruby/data-with-ivars:ClassName
tags (a mapping withmembers
andivars
mappings). LikeMarshal
, this usesrb_struct_initialize
to assign member values to newData
objects.Originally (in the first commit), I updated
Visitor::ToRuby
to simply calldata.send(:initialize, **members)
. But#initialize
freezes the object, so we can't set instance variables afterwards.The second commit sets instance variables before calling
#initialize
, which I think will lead to unhappy surprises for some users. Unfortunately,Data
doesn't give us any other mechanism for setting the members... except for the C API.The third commit works around the above issue by copying how Marshal loads
Data
objects, delegatingToRuby#init_struct(obj, members)
torb_struct_initialize
. I think this is the least surprising behavior for users. This also bypasses any user-defined#initialize
method (which may be seen as a feature or a bug).