Conversation
Some refactoring
|
Please check the syntax and re-target against jruby-9.4 branch. Thank you for your help! |
|
I'm not sure this is exactly the right fix. Looking through all callers, it seems like they all actually just pass in |
|
Fixed lost bracket. I'll try to trace back this call, it seems, that you maybe right. At the first glance i've found to call: and one with int, another with Integer. Hope i'll be back with more complex solution |
|
Here is new proposal, it passed tests in 9.4.10.0, but failing on master (10.0.1.0). Will try clean master to check it |
|
Well, clean tests (without my patch) showed the same picture. So i'm sure patch is not related |
@ngr-ilmarh Are you sharing a gem home with CRuby? The rbs gem does not support JRuby and could not have been installed on JRuby, so I suspect you are using the same gem home for both JRuby and CRuby. This may work ok but is not recommended, since some gems supported by CRuby will not work on JRuby (as shown by these warnings). The warnings may just be throwing of your test results. As for the changes, I can't find no reason this parameter needs to be Integer instead of int, so this is probably fine to merge. I'm going to do a search to see if any third-party code uses this API with Integer. |
|
The only references I could find were in other JRuby forks, so I think this is fine to merge. I'll let the CI tests run and then we can go forward with it. |
|
I realized this was never re-targeted at the |
|
Yes, had ruby installed. And as i see there is date formating error in https://github.com/jruby/jruby/blob/master/test/mri/psych/visitors/test_yaml_tree.rb#L92. i'm sure this patch didn't affect it |
Some refactoring for #8809