add keyword arg support to callMethod directly - #9403
Conversation
|
I like this and I also like the PR only pushes this into embedding API itself. We have a long running task to remove callInfo and to make kwargs more first class and not based on some out-of-band field set per thread. My only thing I wonder about is that Map<String, Object> would somehow be nicer if it could be String or RubySymbol for people who already know they are directly interacting with Ruby from an embedded context. Not sure I am recommending another set of overloads here or whether that case is solved by our larger effort to rewrite kwargs but it seems like a reasonable use case (also it probably could be added as a later PR). |
|
Yes, this seems like a reasonable addition. @enebo To your point, we could use Map<CharSequence, Object> since RubySymbol and RubyString also implement CharSequence. Dunno if it's worth it or not. I'll review. |
headius
left a comment
There was a problem hiding this comment.
Generally on the right track. I think it's worth discussing whether it's safer/cleaner to just add new method names for the kwargs calls rather than more complicated overloads that might be ambiguous in some cases.
…port_to_call_method
…ub.com/drzaiusx11/jruby into add_keyword_arg_support_to_call_method # Conflicts: # core/src/main/java/org/jruby/embed/EmbedRubyObjectAdapter.java # core/src/main/java/org/jruby/embed/ScriptingContainer.java # core/src/main/java/org/jruby/embed/internal/EmbedRubyObjectAdapterImpl.java # core/src/test/java/org/jruby/embed/ScriptingContainerTest.java
|
ok pushed the updates
I'm also purposefully avoiding using 'Object...' for now in the new APIs, opting for arrays to avoid the inevitable disambiguation issues discussed. We can add N-arg positional overloads for N...4 or something sane if thats more ergonomic but I think arrays isn't a terrible first pass |
|
Hey I missed the re-review but I'll take a look tonight. |
headius
left a comment
There was a problem hiding this comment.
Only one small change (same change in several places) needed and this can merge for 10.1.2.0.
…ub.com/drzaiusx11/jruby into add_keyword_arg_support_to_call_method
|
Thank you for your help on this! I am not sure if we've ever chatted about your use of JRuby, but I wanted to mention this in case it works for your use of JRuby: My work on JRuby is now 100% funded by support contracts from Headius Enterprises. We offer customizable packages for JRuby users, with the middle-tier Expert Support being the most popular. Expert Support gives you a couple of dedicated hours of my time per month, prioritized bug fixes, security SLAs, and a whole lot of peace-of-mind that your application will keep running and getting better. We also offer a baseline Pro Support level on a per-seat basis and higher levels of support for codebases under NDA, performance profiling, debugging help, and more. If you have a mission-critical use of JRuby, I'd recommend the Expert Support level, and I'd love to jump on a call to talk about how we can support you. Please consider partnering with us to keep your app running well and the JRuby community moving forward! |
started a discussion here: #9402, copying verbatim here for reference:
I've been using JRuby to wrap some pre-existing Gems to be called by Java using the ScriptingContainer interface provided by JRuby 10.x, however, whenever a Gem uses keyword arguments there doesn't seem to be a clean way to force a Map to be treated as the keyword arguments for that Ruby API. I see theres a ThreadContext.CALL_KEYWORD flag I can use, but it seems more an internal implementation detail than something that should be exposed to scripting users. Afaict this works:
What I'm proposing is adding further callMethod method overloads to include a last argument of Map<String, Object> or similar that will become those keyword arguments (convert strings to symbols automatically, call convertJavaToUsableRubyObject() on values, etc. My guess on why this hasn't come up before is that any scripting use cases predated Ruby's 3.2 order argument changes around auto-conversion of the last hash into keyword args. Since 3.2+ that behavior is disallowed so some other mechanism needs to exist going forward. I think what I propose makes sense here, but i could be missing something important.
This is my attempt at implementing it