Treat value classes as such #610
Conversation
Codecov Report
@@ Coverage Diff @@
## main #610 +/- ##
==========================================
- Coverage 96.11% 94.89% -1.23%
==========================================
Files 48 48
Lines 1649 1724 +75
Branches 152 177 +25
==========================================
+ Hits 1585 1636 +51
- Misses 64 88 +24
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
This actually affects all modules. |
| val v: Value = new StringValue("Hello, world") | ||
| val a = new MapValue(Map("vc" -> v).asJava) | ||
| val c = vt.from(a) | ||
| assert(c == HasValueClass(ValueClass("Hello, world"))) |
There was a problem hiding this comment.
I think this part is useless (it is already tested in the test while doing the round-trip)
There was a problem hiding this comment.
Not really, since the roundtrip will not test the absence of sub-property in the serialized version. Without the change, it will serialize has { "vs": { "str": "Hello, world" } } and deserialize to match the original, and the test will pass, even if the case class is an AnyVal.
There was a problem hiding this comment.
What I mean, is that in the line above is enough
assert(record.get("vc").asString() == "String")We are checking that the value class does not show in the target model.
Since test checks the round trip, we know that the conversion back to scala will create the value class in the HasValueClass
There was a problem hiding this comment.
Oh, yeah, the last assert may be seen as redundant with test + the previous assert. I would be in favor of testing this explicitely, however that's your call.
RustedBones
changed the title
Co-authored-by: Michel Davit <michel@davit.fr>
Agreed, although for others it will be a breaking change. For Neo4J (and maybe some other databases) it won't really change something, since |
|
Other modules actually have the same behavior, considering value classes as record. |
|
@RustedBones does it mean the PR will have to wait until all cases support it? |
|
I'm preparing an update for that. I'm almost done. Would it be fine to continue on your branch? |
Sure! |
|
With this change, |
|
Note: Scala 2.13.9 broke binary compatibility on case classes that are value classes, so any update on it should go directly from Scala 2.13.8 to 2.13.10. See scala/scala#10155 |
The new Neo4J support works well, but value classes generate inner properties in Neo4J, when we would like them to be just strongly typed values.