Copilot commented on code in PR #629:
URL: https://github.com/apache/commons-csv/pull/629#discussion_r3665163030
##########
src/test/java/org/apache/commons/csv/CSVPrinterTest.java:
##########
@@ -1721,6 +1729,29 @@ void testPrinter7() throws IOException {
}
}
+ @Test
+ void testPrintNullValueStartingRecord() throws IOException {
Review Comment:
Given the regression is about ‘leading null renders empty -> blank line ->
dropped by ignoreEmptyLines’, it would be valuable to add an assertion for
`QuoteMode.ALL` here as well. That both documents the intended behavior for
‘quote all’ with null/empty values and will catch the remaining dropped-record
scenario if `QuoteMode.ALL` isn’t handled in the runtime fix.
##########
src/test/java/org/apache/commons/csv/CSVPrinterTest.java:
##########
@@ -1721,6 +1729,29 @@ void testPrinter7() throws IOException {
}
}
+ @Test
+ void testPrintNullValueStartingRecord() throws IOException {
+ final StringWriter sw = new StringWriter();
+ try (CSVPrinter printer = new CSVPrinter(sw, CSVFormat.DEFAULT)) {
+ printer.printRecord("a");
+ printer.printRecord((Object) null);
+ printer.printRecord("b");
+ }
+ final String csv = sw.toString();
+ assertEquals("a" + RECORD_SEPARATOR + "\"\"" + RECORD_SEPARATOR + "b"
+ RECORD_SEPARATOR, csv);
+ try (CSVParser parser = CSVParser.parse(csv, CSVFormat.DEFAULT)) {
+ final List<CSVRecord> records = parser.getRecords();
+ assertEquals(3, records.size());
+ assertArrayEquals(new String[] { "" }, records.get(1).values());
+ }
+ // An explicit MINIMAL quote mode encapsulates it too.
+ assertEquals("\"\"" + RECORD_SEPARATOR,
printNullRecord(CSVFormat.DEFAULT.builder().setQuoteMode(QuoteMode.MINIMAL).get()));
+ // ALL_NON_NULL encodes null as the bare empty field, so it must stay
unquoted.
+ assertEquals(RECORD_SEPARATOR,
printNullRecord(CSVFormat.DEFAULT.builder().setQuoteMode(QuoteMode.ALL_NON_NULL).get()));
+ // Without a quote character there is nothing to encapsulate with.
+ assertEquals(RECORD_SEPARATOR,
printNullRecord(CSVFormat.DEFAULT.builder().setQuote(null).get()));
Review Comment:
Given the regression is about ‘leading null renders empty -> blank line ->
dropped by ignoreEmptyLines’, it would be valuable to add an assertion for
`QuoteMode.ALL` here as well. That both documents the intended behavior for
‘quote all’ with null/empty values and will catch the remaining dropped-record
scenario if `QuoteMode.ALL` isn’t handled in the runtime fix.
##########
src/main/java/org/apache/commons/csv/CSVFormat.java:
##########
@@ -2272,7 +2281,17 @@ private void print(final Object object, final
CharSequence value, final Appendab
out.append(getDelimiterString());
}
if (object == null) {
- out.append(value);
+ if (len == 0 && newRecord && isQuoteCharacterSet() &&
isMinimalQuoteMode()) {
+ // Encapsulate like printWithQuotes does for an empty
value that starts a record: an
+ // unquoted one makes the whole line empty, and a parser
with ignoreEmptyLines enabled
+ // then drops the record. ALL_NON_NULL and NON_NUMERIC are
excluded because they encode
+ // null as the bare empty field.
+ final char quoteChar = quoteCharacter.charValue(); //
Explicit unboxing is intentional
+ out.append(quoteChar);
+ out.append(quoteChar);
+ } else {
+ out.append(value);
+ }
Review Comment:
This fixes the dropped-record case for MINIMAL/unset, but the same
dropped-record behavior can still occur for `QuoteMode.ALL` (which conceptually
should quote empty fields too). With `quoteMode == ALL`, a leading null that
renders empty will still be output as a blank line and can be dropped when
parsing with `ignoreEmptyLines`. Consider extending the condition to also
encapsulate for `QuoteMode.ALL` (while still excluding
`ALL_NON_NULL`/`NON_NUMERIC` as intended), or otherwise aligning null-handling
with the quote-mode semantics so ‘quote all’ doesn’t emit an empty record as an
empty line.
##########
src/test/java/org/apache/commons/csv/CSVPrinterTest.java:
##########
@@ -1854,8 +1885,8 @@ void testPrintRecordsWithObjectArray() throws IOException
{
printer.printRecords(objectArray);
assertEquals(objectArray.length, printer.getRecordCount());
}
- assertEquals(6, charArrayWriter.size());
- assertEquals("\n\n\n\n\n\n", charArrayWriter.toString());
+ assertEquals(16, charArrayWriter.size());
+ assertEquals("\"\"\n\"\"\n\"\"\n\n\"\"\n\"\"\n",
charArrayWriter.toString());
Review Comment:
The `16` here is a magic number that duplicates the information already
encoded by the expected string, which can make future changes more error-prone
(string changes without updating the size). Consider deriving the expected size
from the expected string (e.g., compare `charArrayWriter.size()` to
`expected.length()`), or dropping the separate size assertion if the full
string assertion is sufficient.
--
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]
