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

Replace play.libs.F.Option with Java 8 equivalent #4692

Merged
merged 1 commit into from
Jun 23, 2015
Merged

Replace play.libs.F.Option with Java 8 equivalent #4692

merged 1 commit into from
Jun 23, 2015

Conversation

marcospereira
Copy link
Member

Replace play.libs.F.Option and friends with java.util.Optional and friends

@marcospereira marcospereira self-assigned this Jun 17, 2015
@jroper jroper modified the milestone: 2.5.0 Jun 18, 2015
@marcospereira
Copy link
Member

Here is what I have changed:

  1. Code documentation was also updated to remove mentions about F.Option and subclasses of it.
  2. The Some class, which was a subclass of F.Option did not complain about null values. Because of that, Optional.ofNullable was used to guarantee the same behavior.
  3. Some other small changes were made to cleanup the code, following the boy scout rule defined in contribution guidelines.

@dotta there is a problem with the FormSpec, the build will show that. Looks like the form binding is not handling Optional properly. It could be something related with Spring Binder, but I did not had time to investigate further. I'm submitting the PR so that it can be reviewed sooner.

case x: play.libs.F.None[_] => None
/** Transforms a Play Java `Optional` to a proper Scala `Option`. */
implicit def javaOptionToScala[T](x: Optional[T]): Option[T] = {
if (x.isPresent) Some(x.get) else None
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to use the scala-java8-compat library for these kind of conversions? See here.

@dotta
Copy link
Contributor Author

dotta commented Jun 19, 2015

@marcospereira Great job Marcos!

@marcospereira
Copy link
Member

@dotta I've change the PR to use scala-java8-compat. Thanks for pointing that. Anyway, I could not figure out why play.data.FormSpec is failing. Any help here will be appreciated.

@dotta
Copy link
Contributor Author

dotta commented Jun 22, 2015

@marcospereira LGTM!

Later today I'll see if I can find some time to look into the FormSpec failing test.

@marcospereira
Copy link
Member

Ok. I debug the code and figure out the problem now. F.Option extends java.util.Collection and then, our custom GenericConversion was able to find a TypeDescriptor because Spring understands java.util.Collection but not java.util.Optional. From TypeDescriptor. getElementTypeDescriptor():

If this type is an array, returns the array's component type. If this type is a Collection and it is parameterized, returns the Collection's element type. If the Collection is not parameterized, returns null indicating the element type is not declared.

Returns:
the array component type or Collection element type, or null if this type is a Collection but its element type is not parameterized

Throws:
IllegalStateException - if this type is not a java.util.Collection or array type

And this method was being called here:

https://github.com/playframework/playframework/blob/master/framework/src/play-java/src/main/java/play/data/format/Formatters.java#L171

This is now fixed and the build should be green.

@marcospereira
Copy link
Member

Rebased.

1. Code documentation was also updated to remove mentions about
   F.Option and subclasses of it.
2. The Some class, which was a subclass of F.Option did not complain
   about null values. Because of that, Optional.ofNullable was used
   to guarantee the same behavior.
3. Some other small changes were made to cleanup the code, following
   the boy scout rule defined in contribution guidelines.
jroper added a commit that referenced this pull request Jun 23, 2015
Replace `play.libs.F.Option` with Java 8 equivalent
@jroper jroper merged commit 8b6c824 into playframework:master Jun 23, 2015
@@ -234,7 +234,7 @@ object PlayBuild extends Build {
.enablePlugins(SbtTwirl)
.settings(
addScalaModules(scalaParserCombinators),
libraryDependencies ++= runtime(scalaVersion.value) ++ scalacheckDependencies,
libraryDependencies ++= runtime(scalaVersion.value) ++ scalacheckDependencies ++ Seq(scalaJava8Compat),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a minor nitpick, but you could use :+ (append) instead of ++ and wrapping the dependencies in a Seq. No need to change this, but I'm pointing it out just in case you didn't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not. Thanks. :-)

@dotta
Copy link
Contributor Author

dotta commented Jun 23, 2015

This is now fixed and the build should be green.
(extracted from #4692 (comment))

@marcospereira How did you fix it? I can't easily see what is that was changed to fix it.

@marcospereira
Copy link
Member

@dotta mostly debugging the code and reading Spring docs. Here is the line that did the trick:

https://github.com/playframework/playframework/blob/master/framework/src/play-java/src/main/java/play/data/format/Formatters.java#L168

Before this commit, it was using targetType.getElementTypeDescriptor(), which does works for java.util.Collection (F.Option was a java.util.Collection), but not for java.util.Optional. targetType.elementTypeDescriptor(source) works because it properly creates a TypeDescriptor for Optional.

@dotta
Copy link
Contributor Author

dotta commented Jun 23, 2015

@dotta mostly debugging the code and reading Spring docs. Here is the line that did the trick:

https://github.com/playframework/playframework/blob/master/framework/src/play-java/src/main/java/play/data/format/Formatters.java#L168

Before this commit, it was using targetType.getElementTypeDescriptor(), which does works for java.util.Collection (F.Option was a java.util.Collection), but not for java.util.Optional. targetType.elementTypeDescriptor(source) works because it properly creates a TypeDescriptor for Optional.

Got it, thanks! (Wow, that must have been a tricky one to nail down ;-))

@marcospereira marcospereira deleted the issues/4692 branch June 23, 2015 23:03
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.

None yet

4 participants