Copilot commented on code in PR #2534:
URL: https://github.com/apache/groovy/pull/2534#discussion_r3245949375
##########
subprojects/groovy-csv/src/main/java/groovy/csv/CsvSlurper.java:
##########
@@ -58,7 +61,15 @@ public class CsvSlurper {
* and treats the first row as headers.
*/
public CsvSlurper() {
- this.mapper = new CsvMapper();
+ this.mapper = mapper();
+ }
+
+ static CsvMapper mapper() {
+ return CsvMapper.builder()
+ .addModule(new JavaTimeModule())
+ .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
Review Comment:
This mapper still leaves Jackson's
`SerializationFeature.WRITE_DATES_WITH_CONTEXT_TIME_ZONE` at its default, so
`OffsetDateTime`/zoned values can be normalized to the mapper context time zone
during `CsvBuilder.toCsv` before `parseAs` reads them back. That contradicts
the documented `OffsetDateTime` fidelity for non-UTC offsets; disable the
serialization-side context time-zone adjustment as well or narrow the
documented support.
##########
subprojects/groovy-csv/build.gradle:
##########
@@ -24,6 +24,7 @@ dependencies {
api rootProject
implementation
"com.fasterxml.jackson.dataformat:jackson-dataformat-csv:${versions.jackson}"
implementation
"com.fasterxml.jackson.core:jackson-databind:${versions.jackson}"
+ implementation
"com.fasterxml.jackson.datatype:jackson-datatype-jsr310:${versions.jackson}"
Review Comment:
Adding this new external artifact also needs an entry in
`gradle/verification-metadata.xml`. The repository has Gradle dependency
verification enabled and its metadata currently has no
`jackson-datatype-jsr310` component, so builds that resolve `groovy-csv` will
fail verification until the metadata is updated.
##########
subprojects/groovy-csv/src/spec/doc/csv-userguide.adoc:
##########
@@ -78,6 +78,15 @@
include::../test/groovy/csv/CsvSlurperTest.groovy[tags=typed_parsing,indent=0]
include::../test/groovy/csv/CsvSlurperTest.groovy[tags=typed_parsing_usage,indent=0]
----
+[NOTE]
+====
+CSV has no native types — every cell is text, so the untyped
`parse`/`parseText`
+API always returns `String` values, including for date/time-looking columns.
Use
+the typed `parseAs` API with `java.time.*` fields (see <<csv_typed_temporal>>)
for
+`java.time.LocalDate`, `java.time.LocalTime`, `java.time.LocalDateTime`, and
+`java.time.OffsetDateTime` fidelity.
Review Comment:
The guide promises `OffsetDateTime` fidelity, but the current mapper
configuration still allows serialization through the context time zone, which
can change the original offset before parsing. Either adjust the mapper to
preserve offsets or avoid documenting `OffsetDateTime` as fully faithful.
##########
subprojects/groovy-csv/src/spec/test/groovy/csv/CsvBuilderTest.groovy:
##########
@@ -106,4 +106,45 @@ class CsvBuilderTest {
assert parsed[0].name == 'Widget'
assert parsed[0].price == 9.99
}
+
+ // tag::temporal_class[]
+ static class Event {
+ java.time.LocalDate day
+ java.time.LocalTime windowStart
+ String label
Review Comment:
The temporal round-trip test only exercises `LocalDate` and `LocalTime`,
while the new guide explicitly documents `LocalDateTime` and `OffsetDateTime`
fidelity and the mapper changes include timezone-sensitive configuration.
Please add coverage for at least `LocalDateTime` and a non-UTC `OffsetDateTime`
so the documented behavior is protected.
##########
subprojects/groovy-csv/build.gradle:
##########
@@ -24,6 +24,7 @@ dependencies {
api rootProject
implementation
"com.fasterxml.jackson.dataformat:jackson-dataformat-csv:${versions.jackson}"
implementation
"com.fasterxml.jackson.core:jackson-databind:${versions.jackson}"
Review Comment:
This adds a new runtime dependency. Repository guidance requires new runtime
dependencies to have the dev@ discussion flagged and a NOTICE/LICENSE impact
review, with any required updates included in the same change (see
AGENTS.md:90-91 and .agents/skills/groovy-build/SKILL.md:248-250). Please add
that evidence or the required notice/license updates.
--
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]
