Skip to content
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

Target of javaPrimitiveWrap in WrapFactory includes not primitive wrapper classes. #295

Closed
hatanaka-akihiro opened this issue Jan 24, 2017 · 7 comments

Comments

@hatanaka-akihiro
Copy link

WrapFactory with javaPrimitiveWrap = false does not wrap

  • java.lang.String
  • java.lang.Character
  • subclasses of java.lang.Number
  • java.lang.Boolean

I think "subclasses of java.lang.Number" is problem.
Because subclasses of java.lang.Number includes "not primitive wrapper classes" such as java.math.BigDecimal, java.math.BigInteger.

I would like to expect "not primive wrapper classes".
I think suitable javaPrimitiveWrap target is below.

  • java.lang.String
  • java.lang.Character
  • java.lang.Byte
  • java.lang.Short
  • java.lang.Integer
  • java.lang.Long
  • java.lang.Float
  • java.lang.Double
  • java.lang.Boolean

regards.

@gbrail
Copy link
Collaborator

gbrail commented Sep 22, 2017

So are you suggesting that we change the target to include those specific classes?

That makes sense to me. However I don't know what this might break...

@hatanaka-akihiro
Copy link
Author

I would like to exclude java.math.BigDecimal and java.math.BigInteger from target of javaPrimitiveWrap.

In javascript, I want to avoid being aware of java.lang.String.

I think I can use javaPrimitiveWrap to this purpose.

However, I cannot make javaPrimitiveWrap false, because javaPrimitiveWrap targets java.math.BigDecimal and java.math.BigInteger.

@gbrail
Copy link
Collaborator

gbrail commented Sep 25, 2017 via email

@hatanaka-akihiro
Copy link
Author

I wrote Test class.

RhinoTest.java.txt

I think there is sense of unity in case of javaPrimitiveWrap = false.

@gbrail
Copy link
Collaborator

gbrail commented Sep 27, 2017

Thanks for the test case -- that makes it much clearer. Here is a PR that includes a fix, plus your test code.

#339

Unfortunately the decision as to the result of "typeof" on a BigInteger or BigNumber is happening somewhere else, not in WrapFactory. I'm not sure where that is yet, or whether changing it also would cause big problems for existing code -- I'm kind of inclined to say that we shouldn't change that now.

Please let me know if it's OK to include your test code in the project.

@hatanaka-akihiro
Copy link
Author

Of cause, OK. Please use my code.

@gbrail
Copy link
Collaborator

gbrail commented Sep 28, 2017

OK -- fixed that. I'm not sure about the other problem -- BigInteger instances being shown as a "number" type - that may be a more intrusive change.

#339

@gbrail gbrail closed this as completed Sep 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants