Abacn commented on code in PR #31853:
URL: https://github.com/apache/beam/pull/31853#discussion_r1674554683
##########
sdks/java/io/csv/src/test/java/org/apache/beam/sdk/io/csv/CsvIOParseHelpersTest.java:
##########
@@ -33,6 +34,77 @@
@RunWith(JUnit4.class)
public class CsvIOParseHelpersTest {
+ /** Tests for {@link CsvIOParseHelpers#validate(CSVFormat)}. */
+ @Test
+ public void givenCSVFormatWithHeader_validates() {
+ CSVFormat format = csvFormatWithHeader();
+ CsvIOParseHelpers.validate(format);
+ }
+
+ @Test
+ public void givenCSVFormatWithNullHeader_throwsException() {
+ CSVFormat format = csvFormat();
+ String gotMessage =
+ assertThrows(IllegalArgumentException.class, () ->
CsvIOParseHelpers.validate(format))
+ .getMessage();
+ assertEquals("Illegal class org.apache.commons.csv.CSVFormat: header is
required", gotMessage);
Review Comment:
It's good to check error message. In general, there is no guarantee that
upstream error message will keep the same for version upgrade.
A less strict check is assertThat(gotMessage, contains("header is required"))
##########
sdks/java/io/csv/src/main/java/org/apache/beam/sdk/io/csv/CsvIO.java:
##########
@@ -55,6 +55,68 @@
* <p>Reading from CSV files is not yet implemented. Please see <a
*
href="https://github.com/apache/beam/issues/24552";>https://github.com/apache/beam/issues/24552</a>.
*
+ * <h3>Valid CSVFormat Configuration</h3>
+ *
+ * <p></>A <a
Review Comment:
this list sounds very specific, imo it's better to put it into the javadoc
of CsvIO.Read than top level javadoc here. For now we can put it into the
javadoc of CsvIOParseHelper.validate, as this defines the spec of validate
method.
@damondouglas what do you think?
--
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]
