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/02/24 08:10:37 UTC
[GitHub] [commons-csv] dota17 opened a new pull request #60: Fix csv 149 195
dota17 opened a new pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60
fix [CSV-195] and [CSV-149]
when stream end with normal char but not lineseparator will add 1 to currentlinenumber.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] coveralls edited a comment on issue #60: Fix CSV-149
and CSV-195
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-590207217
[](https://coveralls.io/builds/29654590)
Coverage increased (+0.004%) to 96.421% when pulling **c01aed645056a43d0fe5257b67ab51d03b1423a9 on dota17:fix_csv_149_195** into **96b8485176fab410d6ff829e7e9b6b5f817671ab 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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on a change in pull request #60:
Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383817436
##########
File path: src/test/java/org/apache/commons/csv/CSVParserTest.java
##########
@@ -1281,4 +1283,37 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+
+ @Test
+ public void parseIssue195andIssue149() throws IOException {
+ String records = "A,B,C,D\r\n"
+ + "a1,b1,c1,d1\r\n"
+ + "a2,b2,c2,d2";
+ CSVFormat format = CSVFormat.RFC4180;
+ format = format.withFirstRecordAsHeader();
+ format = format.withIgnoreEmptyLines();
+ format = format.withTrim();
+ format = format.withQuote('"');
+ CSVParser parser = format.parse(new StringReader(records));
Review comment:
Maybe " try-with-resources" instead?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on a change in pull request
#60: Fix CSV-149 and CSV-195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r398591805
##########
File path: src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
##########
@@ -88,11 +89,13 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
for (int i = offset; i < offset + len; i++) {
final char ch = buf[i];
+ final int previous = i > 0 ? buf[i - 1] : lastChar;
if (ch == LF) {
- if (CR != (i > 0 ? buf[i - 1] : lastChar)) {
+ if (CR != previous) {
eolCounter++;
}
- } else if (ch == CR) {
+ } else if ((ch == CR)
+ || (ch == END_OF_STREAM && previous != LF && previous != CR)) {
Review comment:
This looks wrong as `ch == END_OF_STREAM` will always be false since a `char` cannot be negative. If you remove `ch == END_OF_STREAM &&` from the line, the test still passes. Either this code is wrong or the test does not cover the intent of this test.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on a change in pull request #60:
Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383816378
##########
File path: src/test/java/org/apache/commons/csv/CSVParserTest.java
##########
@@ -1281,4 +1283,37 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+
+ @Test
+ public void parseIssue195andIssue149() throws IOException {
+ String records = "A,B,C,D\r\n"
+ + "a1,b1,c1,d1\r\n"
+ + "a2,b2,c2,d2";
+ CSVFormat format = CSVFormat.RFC4180;
Review comment:
[Style] might use final for variables?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on issue #60: Fix CSV-149 and
CSV-195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on issue #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-591301762
LGTM
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on a change in pull request #60:
Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383816676
##########
File path: src/test/java/org/apache/commons/csv/CSVParserTest.java
##########
@@ -1281,4 +1283,37 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+
+ @Test
+ public void parseIssue195andIssue149() throws IOException {
Review comment:
Please separate into two independent tests, e.g. if the test fails we don't know which JIRA ticket is affected
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on a change in pull request #60:
Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383817594
##########
File path: src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
##########
@@ -92,7 +93,8 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
if (CR != (i > 0 ? buf[i - 1] : lastChar)) {
eolCounter++;
}
- } else if (ch == CR) {
+ } else if ((ch == CR)
+ || (ch == END_OF_STREAM && (i > 0 ? buf[i - 1] : lastChar) != LF && (i > 0 ? buf[i - 1] : lastChar) != CR)) {
Review comment:
Can we make the code more self-explaining?
* `i > 0 ? buf[i - 1`is used three times in this code snippet
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] coveralls edited a comment on issue #60: Fix CSV-149
and CSV-195
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-590207217
[](https://coveralls.io/builds/29654929)
Coverage increased (+0.004%) to 96.421% when pulling **e9805a4e399ca9fa634508e19b7f031b6ad3e3f3 on dota17:fix_csv_149_195** into **96b8485176fab410d6ff829e7e9b6b5f817671ab 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
With regards,
Apache Git Services
[GitHub] [commons-csv] dota17 commented on a change in pull request #60: Fix
CSV-149 and CSV-195
Posted by GitBox <gi...@apache.org>.
dota17 commented on a change in pull request #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r399069923
##########
File path: src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
##########
@@ -88,11 +89,13 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
for (int i = offset; i < offset + len; i++) {
final char ch = buf[i];
+ final int previous = i > 0 ? buf[i - 1] : lastChar;
if (ch == LF) {
- if (CR != (i > 0 ? buf[i - 1] : lastChar)) {
+ if (CR != previous) {
eolCounter++;
}
- } else if (ch == CR) {
+ } else if ((ch == CR)
+ || (ch == END_OF_STREAM && previous != LF && previous != CR)) {
Review comment:
thanks ，you are right，that code is useless. I have remove 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
With regards,
Apache Git Services
[GitHub] [commons-csv] dota17 commented on issue #60: Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
dota17 commented on issue #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-591212956
@sgoeschl @garydgregory thanks very much for code review suggestions.
I have updated the pr. and CSV-195 and CSV-149 are the same question, so I name testcase for testcsv149 in a single test file.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] sgoeschl commented on a change in pull request #60:
Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
sgoeschl commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383817594
##########
File path: src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
##########
@@ -92,7 +93,8 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
if (CR != (i > 0 ? buf[i - 1] : lastChar)) {
eolCounter++;
}
- } else if (ch == CR) {
+ } else if ((ch == CR)
+ || (ch == END_OF_STREAM && (i > 0 ? buf[i - 1] : lastChar) != LF && (i > 0 ? buf[i - 1] : lastChar) != CR)) {
Review comment:
Can we make the code more self-explaining?
* `i > 0 ? buf[i - 1`is used three times in this code snippet
* I know it is my fault but I'm intellectually challenged to understand the condition :-(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on issue #60: Fix CSV-149 and
CSV-195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-603914184
I will review tonight...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] coveralls commented on issue #60: Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
coveralls commented on issue #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-590207217
[](https://coveralls.io/builds/28913672)
Coverage remained the same at 93.227% when pulling **ba0c3d8a1949148d214da45d89847fa488d11059 on dota17:fix_csv_149_195** into **e503c568a10ae42364a94695b8505bcb4db28969 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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on issue #60: Fix CSV-149 and
CSV-195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on issue #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-603910544
- https://issues.apache.org/jira/browse/CSV-149
- https://issues.apache.org/jira/browse/CSV-195
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] coveralls edited a comment on issue #60: Fix csv 149
195
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#issuecomment-590207217
[](https://coveralls.io/builds/28963666)
Coverage increased (+0.007%) to 93.234% when pulling **ed998db20b5332ae4db643fc8db0093e7edfeb73 on dota17:fix_csv_149_195** into **e503c568a10ae42364a94695b8505bcb4db28969 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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on a change in pull request
#60: Fix csv 149 195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #60: Fix csv 149 195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r383790405
##########
File path: src/test/java/org/apache/commons/csv/CSVParserTest.java
##########
@@ -1281,4 +1283,37 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+
+ @Test
+ public void parseIssue195andIssue149() throws IOException {
+ String records = "A,B,C,D\r\n"
+ + "a1,b1,c1,d1\r\n"
+ + "a2,b2,c2,d2";
+ CSVFormat format = CSVFormat.RFC4180;
+ format = format.withFirstRecordAsHeader();
+ format = format.withIgnoreEmptyLines();
+ format = format.withTrim();
+ format = format.withQuote('"');
+ CSVParser parser = format.parse(new StringReader(records));
+ try {
+ for (CSVRecord record : parser) {
+ System.out.println(record);
+ }
+ assertEquals(3, parser.getCurrentLineNumber());
+ } finally {
+ parser.close();
+ }
+ String records2 = "A,B,C,D\r\n"
+ + "a1,b1,c1,d1\r\n"
+ + "a2,b2,c2,d2\r\n";
+ CSVParser parser2 = format.parse(new StringReader(records2));
+ try {
+ for (CSVRecord record : parser2) {
+ System.out.println(record);
Review comment:
No console output please.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on a change in pull request
#60: Fix CSV-149 and CSV-195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r398587975
##########
File path: src/test/java/org/apache/commons/csv/issues/JiraCsv149Test.java
##########
@@ -0,0 +1,56 @@
+/*
Review comment:
I added a different test to git master that does not contain so much code duplication. Please rebase on master and see my other 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
With regards,
Apache Git Services
[GitHub] [commons-csv] garydgregory commented on a change in pull request
#60: Fix CSV-149 and CSV-195
Posted by GitBox <gi...@apache.org>.
garydgregory commented on a change in pull request #60: Fix CSV-149 and CSV-195
URL: https://github.com/apache/commons-csv/pull/60#discussion_r398591805
##########
File path: src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
##########
@@ -88,11 +89,13 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
for (int i = offset; i < offset + len; i++) {
final char ch = buf[i];
+ final int previous = i > 0 ? buf[i - 1] : lastChar;
if (ch == LF) {
- if (CR != (i > 0 ? buf[i - 1] : lastChar)) {
+ if (CR != previous) {
eolCounter++;
}
- } else if (ch == CR) {
+ } else if ((ch == CR)
+ || (ch == END_OF_STREAM && previous != LF && previous != CR)) {
Review comment:
This looks wrong as `ch == END_OF_STREAM` will always be false since a `char` cannot be negative. If you remove `ch == END_OF_STREAM &&` from the line, the test still passes. Either this code is wrong or the test does not cover the intent of this expression.
----------------------------------------------------------------
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
With regards,
Apache Git Services