Copilot commented on code in PR #626:
URL: https://github.com/apache/commons-csv/pull/626#discussion_r3637767054
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -584,6 +584,22 @@ void testEndOfFileBehaviorExcel() throws Exception {
}
}
+ @ParameterizedTest
+ @EnumSource(value = CSVFormat.Predefined.class, names = { "MySQL",
"PostgreSQLCsv", "PostgreSQLText", "Oracle" })
+ void testEscapedNullStringIsAValue(final CSVFormat.Predefined predefined)
throws Exception {
+ // "\N" is the MySQL and PostgreSQL null marker, "\\N" is the value
"\N", which is what the printer writes for it.
Review Comment:
The test comment is inaccurate: PostgreSQL CSV does not use "\\N" as its
null marker (its predefined null string is empty). Reword the comment so it
only claims "\\N" is the null marker for formats where that is true, while
still explaining what this test is asserting across all selected formats.
##########
src/test/java/org/apache/commons/csv/CSVPrinterTest.java:
##########
@@ -157,13 +157,17 @@ private void doRandom(final CSVFormat format, final int
iter) throws Exception {
}
/**
- * Converts an input CSV array into expected output values, including
NULLs. NULL strings are converted to null values because the parser will convert
- * these strings to null.
+ * Converts an input CSV array into expected output values, including
NULLs. A value equal to the null string is converted to null only when the
printer
+ * writes that value unchanged; when the printer escapes or quotes it, the
parser reads it back as the value it is.
*/
Review Comment:
This Javadoc overstates the behavior: quoting the null string only
disambiguates it from null in the parser's strict quote modes
(ALL_NON_NULL/NON_NUMERIC). Consider rewording so it matches the actual parser
logic (quoting matters only in strict quote mode; escaping matters when an
escape sequence was translated).
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -806,8 +806,10 @@ private String handleNull(final String input) {
final String nullString = format.getNullString();
final boolean strictQuoteMode = isStrictQuoteMode();
if (input.equals(nullString)) {
+ // An escaped value is the null string itself, not the null
marker: printing null writes the null string
+ // verbatim, so "\\N" for nullString "\N" can only have come from
a value that really is "\N".
// nullString = NULL(String), distinguish between "NULL" and NULL
in ALL_NON_NULL or NON_NUMERIC quote mode
- return strictQuoteMode && isQuoted ? input : null;
+ return reusableToken.isEscaped || strictQuoteMode && isQuoted ?
input : null;
Review Comment:
The ternary condition mixes || and && without parentheses, which is easy to
misread and makes future edits error-prone. Adding explicit parentheses would
make the intended precedence unambiguous.
--
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]
