rootvector2 commented on code in PR #629:
URL: https://github.com/apache/commons-csv/pull/629#discussion_r3673490461
##########
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:
Extending this to `ALL` breaks `JiraCsv203Test.testWithoutNullString`
(CSV-203), which pins that `ALL` writes null as the bare empty field so it
stays distinct from a quoted `""`. That's the same encoding rationale as
`ALL_NON_NULL`, so `ALL` stays excluded. Updated the code comment to say so and
added a test assertion pinning it.
##########
src/test/java/org/apache/commons/csv/CSVPrinterTest.java:
##########
@@ -1721,6 +1729,29 @@ void testPrinter7() throws IOException {
}
}
+ @Test
+ void testPrintNullValueStartingRecord() throws IOException {
Review Comment:
Added an `ALL` assertion. It pins the current behavior: null stays the bare
empty field per CSV-203, since quoting it would break the null vs `""`
distinction that `JiraCsv203Test` checks.
##########
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:
Covered by the new `ALL` assertion above, pinning the bare empty field per
CSV-203.
##########
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:
Done, the size assertion is now derived from the expected 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
