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

Backport scala.jdk.OptionConverters to 2.11 / 2.12 #533

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

haukeh
Copy link
Contributor

@haukeh haukeh commented May 19, 2022

This addresses #532

I have simply copied the implementations of OptionConverters and OptionShape from the standard library, including license string, scaladoc (minus references made to javaapi, which doesn't exist here). A quick look at the git history indicated that this has been done before. I'm not sure if this raises any issues with the license or anything else I'm not aware of, so please let me know if there would be a better way to go about this!

@xuwei-k xuwei-k requested a review from lrytz May 21, 2022 02:56
@lrytz
Copy link
Member

lrytz commented Jun 2, 2022

Hi @haukeh, I'm sorry for the delay.

I think java.lang.Optional is available on bot Scala.js and Scala Native. As far as I know they both use the scala re-implementation of the JDK, which contains Optional (https://github.com/scala-js/scala-js/blob/main/javalib/src/main/scala/java/util/Optional.scala).

@haukeh
Copy link
Contributor Author

haukeh commented Jun 3, 2022

Hi @lrytz, thanks for the info! When I first tried adding it to the default source root, some JS tests failed with errors like:

[error] Referring to non-existent method static java.util.OptionalDouble.of(double)java.util.OptionalDouble

Which made me - having literally no idea about scala-js - just assume Optional in general is not available. But now, after taking a closer look, it seems that it's only the primitive Optional wrappers like OptionalDouble etc. that are not available in JS.

So I guess a way to solve this would be creating a "lightweight" version of OptionConverters in the js and native source folders, which only contains the Option[T] stuff?

@haukeh
Copy link
Contributor Author

haukeh commented Jun 3, 2022

Ok, so I just found out that java.util.Optional is available in Scala-Native on the main branch, but it has not been released yet. (https://github.com/scala-native/scala-native/blob/main/javalib/src/main/scala/java/util/Optional.scala)

So for now I opted to create a slim variant of OptionConverters for JS only. Let me know what you think.

@lrytz
Copy link
Member

lrytz commented Jun 13, 2022

I just found out that java.util.Optional is available in Scala-Native on the main branch, but it has not been released yet

cc @WojciechMazur -- is that possibly changing soon?

Otherwise I'm happy to go with the current solution. Thank you @haukeh!

@WojciechMazur
Copy link

@lrytz There should be a new release of Scala Native introducing j.u.Optional later this week or at the beginning of the next one. I'm currently on mini-vacations and cut off from my workstation.
The implementation of Optional is exactly the same as on the Scala.js so it should work just fine

@SethTisue SethTisue marked this pull request as draft June 16, 2022 04:00
@haukeh
Copy link
Contributor Author

haukeh commented Jul 5, 2022

Hi @lrytz, sorry for the delay. As the new version of scala-native has been released in the meantime, I went ahead and updated it and added OptionConverters also for native.

I opted to create a "shared" source root for js and native, as the implementation is exactly the same. While this feels a little weird for one single class, It felt even worse to just plain copy the class. If anyone knows of a better or more elegant way to do this (with sbt-crossproject etc.) I'd be more than happy to improve on this.

@haukeh haukeh marked this pull request as ready for review July 5, 2022 18:59
Copy link
Member

@lrytz lrytz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @haukeh for pushing through!

@lrytz lrytz merged commit 7a1171d into scala:main Jul 7, 2022
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

Successfully merging this pull request may close these issues.

3 participants