You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fesod.apache.org by "BigDataDZ (via GitHub)" <gi...@apache.org> on 2026/09/08 16:53:53 UTC
[PR] feat: add Character and Year converters (#1017) [fesod]
BigDataDZ opened a new pull request, #1087:
URL: https://github.com/apache/fesod/pull/1087
## Purpose of the pull request
Implements two commonly used JDK 8 compatible converters requested by the community task #1017:
`java.lang.Character` and `java.time.Year`. Proposal comments:
https://github.com/apache/fesod/issues/1017#issuecomment-5588544093 and the amendment below it
(`UUID` / `YearMonth` / `Instant` were proposed by other volunteers in parallel and are left to
them).
Related: #1017
## What's changed?
- **`CharacterStringConverter`** (`org.apache.fesod.sheet.converters.charconverter`): bidirectional
`Character` <-> STRING. Reading an empty string yields `null`; strings longer than one character
throw (wrapped into `ExcelDataConvertException` by the framework) instead of being silently
truncated.
- **`YearStringConverter`** (`org.apache.fesod.sheet.converters.year`): bidirectional `Year` <->
STRING, default format `uuuu`, honors `@DateTimeFormat` custom patterns and
`GlobalConfiguration.locale`.
Both follow the existing `*StringConverter` pattern (e.g. `LocalTimeStringConverter`) and are
registered in `DefaultConverterLoader` under `putAllConverter`, `putWriteConverter` and
`putWriteStringConverter`, so they work on read, xlsx write and CSV write alike.
Each converter ships with JUnit 5 unit tests (11 tests total: type keys, round-trips, default and
custom formats, and error cases).
## Checklist
- [x] I have read the [Contributor Guide](https://fesod.apache.org/community/contribution/).
- [x] I have written the necessary doc or comment.
- [x] I have added the necessary unit tests and all cases have passed.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "BigDataDZ (via GitHub)" <gi...@apache.org>.
BigDataDZ commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r4175804085
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/CharacterAndYearFieldRoundTripTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters;
+
+import java.io.File;
+import java.time.Year;
+import java.util.ArrayList;
+import java.util.List;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.apache.fesod.sheet.testkit.base.AbstractExcelTest;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+import org.apache.fesod.sheet.testkit.helpers.RoundTripHelper;
+import org.apache.fesod.sheet.testkit.params.ExcelFormatSource;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedTest;
+
+/**
+ * Field-level round trips for the Character and Year converters across every supported format.
+ * The xlsx/xls legs exercise the wildcard write key ((type, null)); the csv leg exercises the
+ * (type, STRING) key.
Review Comment:
Good catch, agreed the old javadoc only described the write path. Revised in 54ee630: it now covers both directions - on write, xlsx/xls resolve the wildcard key `(type, null)` while CSV forces a STRING target and resolves `(type, STRING)`; on read, every format resolves by the concrete cell type key, here `(type, STRING)`.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r3967880964
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/charconverter/CharacterStringConverter.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.charconverter;
+
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+
+/**
+ * Character and string converter
+ */
+public class CharacterStringConverter implements Converter<Character> {
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Character.class;
+ }
+
+ @Override
+ public CellDataTypeEnum supportExcelTypeKey() {
+ return CellDataTypeEnum.STRING;
+ }
+
+ @Override
+ public Character convertToJavaData(
+ ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ String stringValue = cellData.getStringValue();
+ if (stringValue == null || stringValue.isEmpty()) {
+ return null;
+ }
+ if (stringValue.length() > 1) {
+ throw new IllegalArgumentException(
+ "Can not convert '" + stringValue + "' to a character, the length must be 1");
+ }
+ return stringValue.charAt(0);
+ }
+
+ @Override
+ public WriteCellData<?> convertToExcelData(
+ Character value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ return new WriteCellData<>(String.valueOf(value));
Review Comment:
Please consider directly using `value.toString()`.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "BigDataDZ (via GitHub)" <gi...@apache.org>.
BigDataDZ commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5651273246
Added in faf2207, covering both layers:
1. **`DateUtilsTest`** — direct coverage for `DateUtils#parseYear` / `#format(Year, ...)`:
default `yyyy` parse and format (with `null` and explicit locales), plus a custom pattern
(`uu`).
2. **Locale coverage** — `format(Year.of(2026), "G y", Locale.CHINA)` → `公元 2026` and
`Locale.US` → `AD 2026`, mirrored through the converter in
`YearStringConverterTest#convertToExcelDataRespectsLocaleForEraPatterns` (a
`GlobalConfiguration` with `Locale.CHINA` / `Locale.US` and a `@DateTimeFormat` pattern of
`G y`).
Also adjusted the existing custom-format test: the previous `MM/yyyy` pattern is invalid for a
bare `Year` (no `MonthOfYear` field — `UnsupportedTemporalTypeException`), replaced with
`'Y'yyyy` → `Y2026`.
Targeted tests 79/79, full fesod-sheet suite 933/933 green.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "BigDataDZ (via GitHub)" <gi...@apache.org>.
BigDataDZ commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5689363653
Good catch @nkuprins — that was an oversight, fixed in 7986ca1:
`putWriteConverter(new YearStringConverter())` added alongside `putWriteStringConverter`.
Without it, a `Year` field failed on the xlsx write path with
`ExcelWriteDataConvertException: Can not find 'Converter' support class Year.` (CSV write and
reads were unaffected, which is why the converter-level unit tests did not catch it).
Added a full round-trip regression (`yearFieldRoundTripsThroughFesodSheet` — a `Year` field
written to xlsx and read back): red without the registration, green with it. Full fesod-sheet
suite green (934/934).
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r3968112813
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/year/YearStringConverter.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.year;
+
+import java.time.Year;
+import java.time.format.DateTimeFormatter;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+
+/**
+ * Year and string converter
+ */
+public class YearStringConverter implements Converter<Year> {
+
+ private static final DateTimeFormatter DEFAULT_FORMATTER = DateTimeFormatter.ofPattern("uuuu");
Review Comment:
While "uuuu" can output a minus sign when formatting, it enforces a fixed 4-character width during parsing and will throw an exception on signed years like "-45". As a result, its parsing behavior is practically no different from "yyyy".
If the intent is to support BCE/negative years and variable-length inputs, changing the pattern to "u" would be better. Otherwise, if negative years are not in scope, would it be cleaner to stick with the conventional "yyyy"?
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "BigDataDZ (via GitHub)" <gi...@apache.org>.
BigDataDZ commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5775714080
Both suggestions addressed in 9683bf7:
- The field round trips moved out of the unit test classes into a dedicated integration test,
`CharacterAndYearFieldRoundTripTest` — extends `AbstractExcelTest`, parameterized with
`@ExcelFormatSource` (XLSX / XLS / CSV) and built on `RoundTripHelper#writeAndRead`.
- As a bonus, the parameterization now covers XLS and CSV too — legs the previous
single-format tests did not exercise.
`YearStringConverterTest` / `CharacterStringConverterTest` remain as pure unit tests.
Full fesod-sheet suite green.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "nkuprins (via GitHub)" <gi...@apache.org>.
nkuprins commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5689074676
`Year` is registered with `putWriteStringConverter` but not with `putWriteConverter`?
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r3968054549
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/converters/year/YearStringConverter.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.year;
+
+import java.time.Year;
+import java.time.format.DateTimeFormatter;
+import org.apache.fesod.sheet.converters.Converter;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.metadata.property.ExcelContentProperty;
+
+/**
+ * Year and string converter
+ */
+public class YearStringConverter implements Converter<Year> {
+
+ private static final DateTimeFormatter DEFAULT_FORMATTER = DateTimeFormatter.ofPattern("uuuu");
+
+ @Override
+ public Class<?> supportJavaTypeKey() {
+ return Year.class;
+ }
+
+ @Override
+ public CellDataTypeEnum supportExcelTypeKey() {
+ return CellDataTypeEnum.STRING;
+ }
+
+ @Override
+ public Year convertToJavaData(
+ ReadCellData<?> cellData, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ return Year.parse(cellData.getStringValue(), getFormatter(contentProperty, globalConfiguration));
+ }
+
+ @Override
+ public WriteCellData<?> convertToExcelData(
+ Year value, ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ return new WriteCellData<>(value.format(getFormatter(contentProperty, globalConfiguration)));
+ }
+
+ private static DateTimeFormatter getFormatter(
+ ExcelContentProperty contentProperty, GlobalConfiguration globalConfiguration) {
+ if (contentProperty == null || contentProperty.getDateTimeFormatProperty() == null) {
+ return DEFAULT_FORMATTER;
+ }
+ return DateTimeFormatter.ofPattern(
+ contentProperty.getDateTimeFormatProperty().getFormat(), globalConfiguration.getLocale());
+ }
Review Comment:
Since the caching logic of `DateTimeFormatter` has already been implemented in `DateUtils` through `DATE_TIME_FORMATTER_THREAD_LOCAL`, can we integrate it into `DateUtils` (for example, `DateUtils#parseYear`, `#format(Year...)`)?
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r4036486174
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/charconverter/CharacterStringConverterTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.charconverter;
+
+import java.io.File;
+import java.util.Collections;
+import java.util.List;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.context.AnalysisContext;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.event.AnalysisEventListener;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(Tags.UNIT)
+class CharacterStringConverterTest {
+
+ private static final GlobalConfiguration GLOBAL_CONFIGURATION = new GlobalConfiguration();
+ private final CharacterStringConverter converter = new CharacterStringConverter();
+
+ @Test
+ void supportJavaTypeKey() {
+ Assertions.assertEquals(Character.class, converter.supportJavaTypeKey());
+ }
+
+ @Test
+ void supportExcelTypeKey() {
+ Assertions.assertEquals(CellDataTypeEnum.STRING, converter.supportExcelTypeKey());
+ }
+
+ @Test
+ void convertToJavaDataReturnsSingleCharacter() {
+ Assertions.assertEquals(
+ Character.valueOf('A'),
+ converter.convertToJavaData(new ReadCellData<>("A"), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToJavaDataReturnsNullForEmptyString() {
+ Assertions.assertNull(converter.convertToJavaData(new ReadCellData<>(""), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToJavaDataThrowsOnLongerStrings() {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> converter.convertToJavaData(new ReadCellData<>("AB"), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToExcelDataWritesStringCell() {
+ WriteCellData<?> cellData = converter.convertToExcelData('A', null, GLOBAL_CONFIGURATION);
+ Assertions.assertEquals(CellDataTypeEnum.STRING, cellData.getType());
+ Assertions.assertEquals("A", cellData.getStringValue());
+ }
+
+ @Test
+ void characterFieldRoundTripsThroughFesodSheet() throws Exception {
+ File file = File.createTempFile("fesod-character", ".xlsx");
+ file.deleteOnExit();
+ CharacterWriteData row = new CharacterWriteData();
+ row.setFlag('A');
+
+ FesodSheet.write(file, CharacterWriteData.class).sheet().doWrite(Collections.singletonList(row));
+
+ List<CharacterReadData> rows = FesodSheet.read(file, CharacterReadData.class, new CharacterReadListener())
+ .sheet()
+ .doReadSync();
Review Comment:
Suggest extending `AbstractExcelTest` and using its `createTempFile(...)` helper to cover multiple formats (e.g., XLSX, XLS, CSV). And using `RoundTripHelper#writeAndRead(...)` to simplify the boilerplate read/write code.
Additionally, it would be better to move this test into a dedicated integration test class rather than mixing it with pure unit tests.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5650446124
It is recommended to add tests for the following scenarios to cover the complete behavior of `YearStringConverter` / `DateUtils`:
- `DateUtils#parseYear()` / `format(Year)`
- Locale's impact on formatting（format = "G y"，`Locale.CHINA` -> "公元 2026"，`Locale.US` -> "AD 2026"）
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r3979627955
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/DateUtils.java:
##########
@@ -105,6 +106,7 @@ public class DateUtils {
public static final String DATE_FORMAT_19_FORWARD_SLASH = "yyyy/MM/dd HH:mm:ss";
public static final String TIME_FORMAT_5 = "HH:mm";
public static final String TIME_FORMAT_8 = "HH:mm:ss";
+ public static final String DEFAULT_YEAR_FORMAT = "yyyy";
Review Comment:
Recommended to follow the field naming convention.
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "BigDataDZ (via GitHub)" <gi...@apache.org>.
BigDataDZ commented on PR #1087:
URL: https://github.com/apache/fesod/pull/1087#issuecomment-5619954825
Done in 254a11e — renamed to `DateUtils.defaultYearFormat`, following the sibling fields
`defaultDateFormat` / `defaultLocalDateFormat`, and `parseYear` / `format(Year, ...)` now fall
back to it when no explicit format is given, so it works as a global default just like the other
two. Tests re-run green (11/11 targeted, 927/927 full suite).
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng merged PR #1087:
URL: https://github.com/apache/fesod/pull/1087
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r4036508224
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/charconverter/CharacterStringConverterTest.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters.charconverter;
+
+import java.io.File;
+import java.util.Collections;
+import java.util.List;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fesod.sheet.FesodSheet;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.context.AnalysisContext;
+import org.apache.fesod.sheet.enums.CellDataTypeEnum;
+import org.apache.fesod.sheet.event.AnalysisEventListener;
+import org.apache.fesod.sheet.metadata.GlobalConfiguration;
+import org.apache.fesod.sheet.metadata.data.ReadCellData;
+import org.apache.fesod.sheet.metadata.data.WriteCellData;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(Tags.UNIT)
+class CharacterStringConverterTest {
+
+ private static final GlobalConfiguration GLOBAL_CONFIGURATION = new GlobalConfiguration();
+ private final CharacterStringConverter converter = new CharacterStringConverter();
+
+ @Test
+ void supportJavaTypeKey() {
+ Assertions.assertEquals(Character.class, converter.supportJavaTypeKey());
+ }
+
+ @Test
+ void supportExcelTypeKey() {
+ Assertions.assertEquals(CellDataTypeEnum.STRING, converter.supportExcelTypeKey());
+ }
+
+ @Test
+ void convertToJavaDataReturnsSingleCharacter() {
+ Assertions.assertEquals(
+ Character.valueOf('A'),
+ converter.convertToJavaData(new ReadCellData<>("A"), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToJavaDataReturnsNullForEmptyString() {
+ Assertions.assertNull(converter.convertToJavaData(new ReadCellData<>(""), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToJavaDataThrowsOnLongerStrings() {
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> converter.convertToJavaData(new ReadCellData<>("AB"), null, GLOBAL_CONFIGURATION));
+ }
+
+ @Test
+ void convertToExcelDataWritesStringCell() {
+ WriteCellData<?> cellData = converter.convertToExcelData('A', null, GLOBAL_CONFIGURATION);
+ Assertions.assertEquals(CellDataTypeEnum.STRING, cellData.getType());
+ Assertions.assertEquals("A", cellData.getStringValue());
+ }
+
+ @Test
+ void characterFieldRoundTripsThroughFesodSheet() throws Exception {
+ File file = File.createTempFile("fesod-character", ".xlsx");
+ file.deleteOnExit();
+ CharacterWriteData row = new CharacterWriteData();
+ row.setFlag('A');
+
+ FesodSheet.write(file, CharacterWriteData.class).sheet().doWrite(Collections.singletonList(row));
+
+ List<CharacterReadData> rows = FesodSheet.read(file, CharacterReadData.class, new CharacterReadListener())
+ .sheet()
+ .doReadSync();
Review Comment:
Same advice: `YearStringConverterTest#yearFieldRoundTripsThroughFesodSheet()`
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org
Re: [PR] feat: add Character and Year converters (#1017) [fesod]
Posted by "bengbengbalabalabeng (via GitHub)" <gi...@apache.org>.
bengbengbalabalabeng commented on code in PR #1087:
URL: https://github.com/apache/fesod/pull/1087#discussion_r4071337035
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/converters/CharacterAndYearFieldRoundTripTest.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.converters;
+
+import java.io.File;
+import java.time.Year;
+import java.util.ArrayList;
+import java.util.List;
+import lombok.Getter;
+import lombok.Setter;
+import org.apache.fesod.sheet.annotation.ExcelProperty;
+import org.apache.fesod.sheet.testkit.Tags;
+import org.apache.fesod.sheet.testkit.base.AbstractExcelTest;
+import org.apache.fesod.sheet.testkit.enums.ExcelFormat;
+import org.apache.fesod.sheet.testkit.helpers.RoundTripHelper;
+import org.apache.fesod.sheet.testkit.params.ExcelFormatSource;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.ParameterizedTest;
+
+/**
+ * Field-level round trips for the Character and Year converters across every supported format.
+ * The xlsx/xls legs exercise the wildcard write key ((type, null)); the csv leg exercises the
+ * (type, STRING) key.
Review Comment:
In read scenarios, xlsx/xls also override matching logic of the kind `(JavaType, CellDataType)`, not just `(JavaType, null)`. Therefore, this comment only describes the override situation on the write path and does not cover the behavior of the read path, which can easily lead to misunderstanding.
My personal suggestion is to remove this description entirely (or revise it into a more comprehensive explanation).
--
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: commits-unsubscribe@fesod.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@fesod.apache.org
For additional commands, e-mail: commits-help@fesod.apache.org