Copilot commented on code in PR #2533:
URL: https://github.com/apache/groovy/pull/2533#discussion_r3245943900
##########
subprojects/groovy-yaml/src/main/java/groovy/yaml/YamlSlurper.java:
##########
@@ -125,12 +128,19 @@ public <T> T parseTextAs(Class<T> type, String yaml) {
*/
public <T> T parseAs(Class<T> type, Reader reader) {
try {
- return new ObjectMapper(new YAMLFactory()).readValue(reader, type);
+ return mapper(new YAMLFactory()).readValue(reader, type);
} catch (IOException e) {
throw new YamlRuntimeException(e);
}
}
+ static ObjectMapper mapper(YAMLFactory factory) {
+ return new ObjectMapper(factory)
+ .registerModule(new JavaTimeModule())
+ .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
+
.disable(DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE);
+ }
Review Comment:
`mapper(...)` disables `SerializationFeature.WRITE_DATES_AS_TIMESTAMPS`,
which changes `YamlBuilder.toYaml` output for `java.util.Date`/`Calendar` (and
any other date-like types using default Jackson serializers) from numeric
timestamps to textual ISO-8601 strings. If this is intentional, it should be
called out in the user guide/release notes; if not, consider scoping this
setting to the java.time handling only (or leaving the default for legacy
`java.util.Date`).
##########
subprojects/groovy-yaml/src/spec/doc/yaml-userguide.adoc:
##########
@@ -82,14 +82,23 @@ The following table gives an overview of the YAML types and
the corresponding Gr
|null
|`null`
-|date
-|`java.util.Date` based on the `yyyy-MM-dd'T'HH:mm:ssZ` date format
+|date/time
+|`java.lang.String` (see the note below)
|===
[NOTE]
Whenever a value in YAML is `null`, `YamlSlurper` supplements it with the
Groovy `null` value. This is in contrast to other
YAML parsers that represent a `null` value with a library-provided singleton
object.
+[NOTE]
+====
+For the untyped `parse`/`parseText` API, YAML date and time values are
returned as
+`String`. `YamlSlurper` routes the untyped path through a YAML-to-JSON
conversion,
+and JSON has no native temporal types. Use the typed `parseAs`/`parseTextAs`
API
+(see <<yaml_typed_temporal>>) for `java.time.LocalDate`, `java.time.LocalTime`,
+`java.time.LocalDateTime`, and `java.time.OffsetDateTime` fidelity.
+====
Review Comment:
The note lists `java.time.LocalDateTime` as supported via typed
`parseAs`/`parseTextAs`, but the added spec tests only cover `OffsetDateTime`,
`LocalDate`, and `LocalTime`. To prevent regressions (and keep docs/tests
aligned), consider adding a `LocalDateTime` field to `Event` and asserting it
round-trips as well (or remove it from the list if it isn’t supported).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
