-
Notifications
You must be signed in to change notification settings - Fork 211
Replace C extension ToRuby#init_data with Data#initialize bind_call
#765
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
Replace C extension ToRuby#init_data with Data#initialize bind_call
#765
Conversation
|
I'm +1 for this change but I still think we should disallow anonymous Data types from being dumped. It doesn't do what anyone would expect and will create a new Data class for each such object in the stream. I'll put together a PR. |
d38dade to
6a82669
Compare
|
The instance variable-related tests on JRuby will be fixed by jruby/jruby#9138. Instance variables on Data subtypes is still pretty hacky and only supports immediate children, but the fix here passes all tests on this branch and instance variables on direct subtypes of a Data type work properly. |
|
@headius I deleted that |
|
@nevans Thank you; I believe once my JRuby PR lands this should be green on jruby-head. |
|
I restarted the failed jobs since I believe both JRuby and TR have pushed updated head builds. |
|
Something very strange going on with the JRuby job on Windows but it's very unlikely to be related to this PR. I'll look into it tomorrow. |
eregon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
Nit: the comments in Also if it's only JRuby on Windows failing I'd be inclined to exclude it for now, and reenable it when it's fixed (still better than no JRuby testing at all like on current master). |
|
Re JRuby on Windows the failure is: Which means |
|
Actually rake-compiler is already 1.3.0 which has this PR: |
|
I found the issue: rake-compiler/rake-compiler#247 (comment) |
|
I've rerun the jruby windows job after ruby/setup-ruby#836 so it should be green now, or at least proceed further than |
I pushed a commit for that. |
|
Thanks for the extra fix in setup-ruby! |
See prior discussions here: