You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/12/18 06:53:44 UTC
[GitHub] [commons-csv] arturobernalg opened a new pull request #127: Minor Improvement
arturobernalg opened a new pull request #127:
URL: https://github.com/apache/commons-csv/pull/127
* Add final
* Unnecessary semicolon ''
* Use StandardCharsets
* Fix javadoc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35992932)
Coverage decreased (-0.002%) to 98.454% when pulling **f1fedd67c43359174ab466303063e0f37e4f80d2 on arturobernalg:feature/minor_improvement** into **155a76906a1c65a95ac144cc6aa77b9c9a1cbbfe on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35992907)
Coverage decreased (-0.002%) to 98.454% when pulling **988cd93a85d5ef29e583d8044b0d5314158ddb5c on arturobernalg:feature/minor_improvement** into **155a76906a1c65a95ac144cc6aa77b9c9a1cbbfe on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35992324)
Coverage decreased (-0.002%) to 98.454% when pulling **367a3c607906a71d53a83fcefb03c92b585c338a on arturobernalg:feature/minor_improvement** into **7ebb8db97794986ef958a8b9a50d057940b58105 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545927905
##########
File path: 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)) {
Review comment:
Good one :-)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r550007664
##########
File path: 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
Review comment:
OK, got it. Reverted
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549818761
##########
File path: 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
Review comment:
ok, got it. changed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545812569
##########
File path: 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
Review comment:
"scape"?
##########
File path: 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
Review comment:
Does not match the code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545953753
##########
File path: 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
Review comment:
Sorry Garry, but What is the standard comment?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35825054)
Coverage decreased (-0.002%) to 98.454% when pulling **ab14f47fdef49964484c1d81876caf1b177b7e3f on arturobernalg:feature/minor_improvement** into **7ebb8db97794986ef958a8b9a50d057940b58105 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549815045
##########
File path: 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
Review comment:
@arturobernalg
The JRE uses "if an I/O error occurs." but I prefer "Thrown when an I/O error occurs."
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545954724
##########
File path: 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];
Review comment:
true. changed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/36000827)
Coverage decreased (-0.002%) to 98.454% when pulling **16827955cc02d2d2adf6e12689af2c865c5ce895 on arturobernalg:feature/minor_improvement** into **8e953c0c17a2fd251e30f6c2673da64e18181e7b on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545927053
##########
File path: 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
Review comment:
Might as well use the "standard" comment here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/36000830)
Coverage decreased (-0.002%) to 98.454% when pulling **16827955cc02d2d2adf6e12689af2c865c5ce895 on arturobernalg:feature/minor_improvement** into **8e953c0c17a2fd251e30f6c2673da64e18181e7b on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls commented on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35817535)
Coverage decreased (-0.002%) to 98.454% when pulling **6ba7b2965ddd702858b8a4603c2a42875d280655 on arturobernalg:feature/minor_improvement** into **7ebb8db97794986ef958a8b9a50d057940b58105 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35823677)
Coverage decreased (-0.002%) to 98.454% when pulling **1b41677139303627cd3981086125b26c09ebedac on arturobernalg:feature/minor_improvement** into **7ebb8db97794986ef958a8b9a50d057940b58105 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549818761
##########
File path: 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
Review comment:
ok, got it. Change it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
[](https://coveralls.io/builds/35823642)
Coverage decreased (-0.002%) to 98.454% when pulling **1b41677139303627cd3981086125b26c09ebedac on arturobernalg:feature/minor_improvement** into **7ebb8db97794986ef958a8b9a50d057940b58105 on apache:master**.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549826167
##########
File path: 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
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545919107
##########
File path: 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
Review comment:
changed. What do you thing?=
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545918959
##########
File path: 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
Review comment:
changed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549884810
##########
File path: 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
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] coveralls edited a comment on pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#issuecomment-748003900
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] arturobernalg commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
arturobernalg commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r549830686
##########
File path: 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
Review comment:
OK. Got it. Changed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory merged pull request #127: Minor Improvements
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #127:
URL: https://github.com/apache/commons-csv/pull/127
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a change in pull request #127: Minor Improvement
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #127:
URL: https://github.com/apache/commons-csv/pull/127#discussion_r545927545
##########
File path: 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];
Review comment:
Constants are usually in uppercase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org