Minor Improvements #127

arturobernalg · 2020-12-18T06:53:29Z

Add final
Unnecessary semicolon ''
Use StandardCharsets
Fix javadoc

coveralls · 2020-12-18T10:16:11Z

Coverage decreased (-0.002%) to 98.454% when pulling 1682795 on arturobernalg:feature/minor_improvement into 8e953c0 on apache:master.

garydgregory · 2020-12-18T12:53:31Z

src/test/java/org/apache/commons/csv/CSVParserTest.java

@@ -572,7 +572,7 @@ public void testGetOneLine() throws IOException {
    /**
     * Tests reusing a parser to process new string records one at a time as they are being discovered. See [CSV-110].
     *
-     * @throws IOException
+     * @throws Exception Any exception can be thrown


Does not match the code.

garydgregory · 2020-12-18T12:54:00Z

src/main/java/org/apache/commons/csv/CSVFormat.java

@@ -1629,6 +1629,7 @@ private CharSequence trim(final CharSequence charSequence) {
     * Verifies the consistency of the parameters and throws an IllegalArgumentException if necessary.
     *
     * @throws IllegalArgumentException
+     *             thrown if the delimiter is a scape character


changed. What do you thing?=

garydgregory · 2020-12-18T15:57:13Z

src/test/java/org/apache/commons/csv/CSVParserTest.java

@@ -572,7 +572,7 @@ public void testGetOneLine() throws IOException {
    /**
     * Tests reusing a parser to process new string records one at a time as they are being discovered. See [CSV-110].
     *
-     * @throws IOException
+     * @throws IOException If something goes wrong


Might as well use the "standard" comment here

Sorry Garry, but What is the standard comment?

@arturobernalg
The JRE uses "if an I/O error occurs." but I prefer "Thrown when an I/O error occurs."

ok, got it. changed

garydgregory · 2020-12-18T15:58:03Z

src/test/java/org/apache/commons/csv/PerformanceTest.java

@@ -64,7 +64,7 @@
    private static int max = 11; // skip first test

    private static int num = 0; // number of elapsed times recorded
-    private static long[] elapsedTimes = new long[max];
+    private static final long[] elapsedTimes = new long[max];


Constants are usually in uppercase.

true. changed

garydgregory · 2020-12-18T15:58:42Z

src/test/java/org/apache/commons/csv/CSVFileParserTest.java

@@ -135,7 +136,7 @@ public void testCSVUrl(final File testFile) throws Exception {

            // Now parse the file and compare against the expected results
            final URL resource = ClassLoader.getSystemResource("org/apache/commons/csv/CSVFileParser/" + split[0]);
-            try (final CSVParser parser = CSVParser.parse(resource, Charset.forName("UTF-8"), format)) {
+            try (final CSVParser parser = CSVParser.parse(resource, StandardCharsets.UTF_8, format)) {


Good one :-)

garydgregory · 2020-12-29T19:46:21Z

src/main/java/org/apache/commons/csv/CSVFormat.java

@@ -1628,7 +1628,8 @@ private CharSequence trim(final CharSequence charSequence) {
    /**
     * Verifies the consistency of the parameters and throws an IllegalArgumentException if necessary.
     *
-     * @throws IllegalArgumentException
+     * @throws IllegalArgumentException If something goes wrong


Hi @arturobernalg
This change does not help anyone, and it's confusing when the line above tells you when the exception would be thrown. I've updated git master for this Javadoc. Please rebase on master. TY.

OK. Got it. Changed

garydgregory · 2020-12-29T23:20:54Z

src/test/java/org/apache/commons/csv/CSVParserTest.java

@@ -427,7 +427,7 @@ public void testExcelFormat2() throws Exception {
    /**
     * Tests an exported Excel worksheet with a header row and rows that have more columns than the headers
     *
-     * @throws Exception
+     * @throws Exception Any exception can be thrown


Not helpful. Tests like this one can throw Exception as a convention instead of throwing a potentially long list of exceptions which are not useful and just clutter up the method signatures. So either we should not have a Javadoc tag or if we have one, it should carry proper and useful documentation. In this case, I think that there is nothing interesting to say.

OK, got it. Reverted

* Add final * Unnecessary semicolon '' * Use StandardCharsets * Fix javadoc

garydgregory requested changes Dec 18, 2020

View reviewed changes

arturobernalg requested a review from garydgregory December 18, 2020 15:47

garydgregory reviewed Dec 18, 2020

View reviewed changes

arturobernalg requested a review from garydgregory December 20, 2020 06:18

garydgregory reviewed Dec 29, 2020

View reviewed changes

Minor Improvement:

1682795

* Add final * Unnecessary semicolon '' * Use StandardCharsets * Fix javadoc

arturobernalg requested a review from garydgregory December 30, 2020 07:59

garydgregory changed the title ~~Minor Improvement~~ Minor Improvements Dec 30, 2020

garydgregory merged commit d580c90 into apache:master Dec 30, 2020

asfgit pushed a commit that referenced this pull request Dec 30, 2020

Minor Improvements #127.

895188c

arturobernalg deleted the feature/minor_improvement branch January 24, 2021 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor Improvements #127

Minor Improvements #127

arturobernalg commented Dec 18, 2020

coveralls commented Dec 18, 2020 •

edited

garydgregory Dec 18, 2020

arturobernalg Dec 18, 2020

garydgregory Dec 18, 2020

arturobernalg Dec 18, 2020

garydgregory Dec 18, 2020

arturobernalg Dec 18, 2020

garydgregory Dec 29, 2020

arturobernalg Dec 29, 2020 •

edited

garydgregory Dec 18, 2020

arturobernalg Dec 18, 2020

garydgregory Dec 18, 2020

garydgregory Dec 29, 2020

arturobernalg Dec 29, 2020

garydgregory Dec 29, 2020

arturobernalg Dec 30, 2020

Minor Improvements #127

Minor Improvements #127

Conversation

arturobernalg commented Dec 18, 2020

coveralls commented Dec 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturobernalg Dec 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 18, 2020 •

edited

arturobernalg Dec 29, 2020 •

edited