Quill 1.0.1 support with java.time. #11
Conversation
|
Thanks for the PR! I'm at scala exchange right now, this looks great but I need to time to take a closer look after the conference :) |
|
Very cool. Enjoy London! |
| | * Quill used to have this trait before v1, but it's still useful to keep. | ||
| | * Examples are: pattern matching on wrapped type and conversion to JSON objects. | ||
| | */ | ||
| | trait WrappedValue[T] extends Any with WrappedType { self: AnyVal => |
There was a problem hiding this comment.
Since WrappedValue is static, could we move it somewhere outside the generated code?
- Easiest solution would probably be to put this into a separate source file and link to from the readme for people to copy-paste into their project.
- Another solution is put it into a separate project in the build and publish to Maven.
There was a problem hiding this comment.
A separate file maybe? I think a separate project is a bit overkill for only 7 lines of code. If we generate two files (one dynamic and one static) in the same package, then the generated code is fully stand-alone valid Scala code and users won't need to manually fix imports and such. What do you think?
There was a problem hiding this comment.
We could add a "staticFile: Option[File]" option where we write static contents if the setting is configured. WDYT?
| | * Generated using [[https://github.com/olafurpg/scala-db-codegen scala-db-codegen]] | ||
| | * - Number of tables: ${tables.size} | ||
| | * - Database URL: ${options.url} | ||
| | * - Database schema: ${options.schema} |
| @@ -28,7 +28,7 @@ object TypeMap { | |||
| "varchar" -> "String", | |||
| "serial" -> "Int", | |||
| "bigserial" -> "Long", | |||
| "timestamp" -> "java.util.Date", | |||
| "timestamp" -> "java.time.LocalDateTime", // Since Quill 1.0.1, the scalajs-java-time library supports java time. | |||
There was a problem hiding this comment.
Yay \o/
I haven't tried scalajs-java-time yet, seems to still be a wip. However, I'm happy to change the default since I suspect the scalajs use-case is not important for all quill users. Worst case, it's still still possible to override this in case you want java.util.Date.
There was a problem hiding this comment.
I also don't have any experience with scalajs-java-time. I assume the quill maintainers did this with good reasons and I am very happy with their choice. Just like you said, there is always a way to override the defaults so I think we're safe doing this. 👍
Hi @olafurpg could you please review this PR?
I decided to copy the
WrappedValueto the codegen because there is no other way to identify the type of the wrapped value (correct me if I'm wrong).