-
Notifications
You must be signed in to change notification settings - Fork 846
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
improve java.util.{List,Map} interop #713
Conversation
Thanks! I can see how this will be useful for people who embed Java in Rhino a lot. Whenever I see a pattern in which we introduce a bunch of flags set in the constructor as "isThis" or "isThat", and then many methods start with "if (isThis) { do this } else if (isThat) { doThat }", I find myself asking, "would inheritance be a cleaner way to implement this?" What if:
I think this might also be handy someday because you could easily extend the pattern for other types of classes if people would like. Would that be a painful change? I think it'd make things cleaner in the long run. |
hi @gbrail , thank you for reviewing the PR :)
Agree, I was a little bit unsure which approach would be more acceptable :).
not painful at all :), in fact I had the other variant in another branch (https://github.com/mozilla/rhino/compare/master...syjer:interop-java-list-map?expand=1) :). I'll take your feedback and incorporate the changes :) |
by the way, by I can't find the method |
Yes -- that's the class. Sorry!
…On Tue, Jun 16, 2020 at 2:20 PM Sylvain Jermini ***@***.***> wrote:
by the way, by ScriptRuntime.wrapAsJavaObject you mean
WrapFactory.wrapAsJavaObject right?
I can't find the method wrapAsJavaObject in ScriptRuntime.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#713 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I2YZC3U7HT4MNV5LP3TRW7OYNANCNFSM4N6B3BWQ>
.
|
38c8559
to
6ab94c6
Compare
I've updated the PR following your feedback :) The tests run successfully locally too. |
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.
I think this looks great. I have two minor suggestions -- if you like them, then please give them a try, and otherwise let me know if you'd rather not!
|
||
@Override | ||
public Object[] getIds() { | ||
return map.keySet().toArray(); |
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.
This may be over-complication but JavaScript has taught me to that anything is possible...
Is it possible that a user would use this with a map that has keys that are not strings or integers? If so, then it might aid interop with JavaScript if we iterated through the list and ensured that each key is either a string or an integer by calling ScriptRuntime.toInt32 or ScriptRuntime.toString depending on the type, and letting those methods throw a JavaScript exception if something doesn't work (for instance, if they try to use a Symbol or something).
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.
This may be over-complication but JavaScript has taught me to that anything is possible...
Haha, i feel your pain ;).
Is it possible that a user would use this with a map that has keys that are not strings or integers? If so, then it might aid interop with JavaScript if we iterated through the list and ensured that each key is either a string or an integer by calling ScriptRuntime.toInt32 or ScriptRuntime.toString depending on the type, and letting those methods throw a JavaScript exception if something doesn't work (for instance, if they try to use a Symbol or something).
Ok for ensuring the type :). I'll update the PR with your feedback. Thank you for your help.
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.
Actually, what about filtering out non String/Integer keys instead of converting them?
I've updated the PR incorporating the feedback.
(By the way, I noticed that running it with java 11 |
3afa465
to
bdec5fc
Compare
This looks good now. Thanks! |
thank you for guiding me in the process 👍 |
Yes -- whatever is happening in Java 11 with the Date tests should be fixed. It's possible that Rhino's date handling is no longer valid in Java 11, but it's a lot more possible that the particular tests makes assumptions that are no longer valid and should be replaced by a new test. |
ok, I'll have a look at the failing tests :) |
Hi, this PR is based on #561 to improve the interoperability with
java.util.List
andjava.util.Map
.As a main difference with #561, instead of creating 2 new wrapper, I've modified the
NativeJavaObject
class, as I think it's a little less disruptive and more easily reviewable.It add some additional overhead (2 new boolean fields and some additional checks in has/get/put methods, but I would hope it's acceptable.
(Btw, I've noticed the travis build fail, but locally the tests passes)