You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "gbidsilva (via GitHub)" <gi...@apache.org> on 2023/08/30 11:47:18 UTC
[GitHub] [commons-csv] gbidsilva opened a new pull request, #348: Error message fix
gbidsilva opened a new pull request, #348:
URL: https://github.com/apache/commons-csv/pull/348
When a parsing error occurred during the CSV data parsing, the exception message contains duplicate Exception class name.
Relates to: https://issues.apache.org/jira/browse/CSV-309
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #348: Error message fix
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310162041
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
"Error" has a specific meaning in Java, and we did _not_ catch a `java.lang.Error` here. So saying "Exception reading next record..." is more accurate than "Error...", so then the difference is whether the sentence start with "Exception ..." vs. "IOException..." (or a subclass), so the difference is not interesting IMO. Or, we could say `"CSVParser#nextRecord(): " + e` where the (maybe dubious) benefit is that it is a touch more language-neutral.
Thoughts?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] codecov-commenter commented on pull request #348: Error message fix
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #348:
URL: https://github.com/apache/commons-csv/pull/348#issuecomment-1699091456
## [Codecov](https://app.codecov.io/gh/apache/commons-csv/pull/348?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#348](https://app.codecov.io/gh/apache/commons-csv/pull/348?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (dbc418b) into [master](https://app.codecov.io/gh/apache/commons-csv/commit/9668d4628eecb44f57d1107adece8f954054cbc4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9668d46) will **not change** coverage.
> Report is 19 commits behind head on master.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #348 +/- ##
=========================================
Coverage 97.87% 97.87%
Complexity 549 549
=========================================
Files 11 11
Lines 1178 1178
Branches 204 204
=========================================
Hits 1153 1153
Misses 13 13
Partials 12 12
```
| [Files Changed](https://app.codecov.io/gh/apache/commons-csv/pull/348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...rc/main/java/org/apache/commons/csv/CSVParser.java](https://app.codecov.io/gh/apache/commons-csv/pull/348?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY3N2L0NTVlBhcnNlci5qYXZh) | `96.87% <100.00%> (ø)` | |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/commons-csv/pull/348/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #348: Error message fix
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310172501
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
It's not an `Error`... See below.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory merged pull request #348: Error message fix
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #348:
URL: https://github.com/apache/commons-csv/pull/348
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] michael-o commented on a diff in pull request #348: Error message fix
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310208954
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
Why not just: "Failed to read next record"?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #348: Error message fix
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310162041
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
"Error" has a specific meaning in Java, and we did _not_ catch a `java.lang.Error` here. I'll adjust.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #348: Error message fix
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310163383
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
Might be the intension was to provide the sense of error in the message itself.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #348: Error message fix
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310163383
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
Might be the intention was to provide the sense of error in the message itself.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #348: Error message fix
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310162041
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
"Error" has a specific meaning in Java, and we did _not_ catch a `java.lang.Error` here. Saying "Exception reading next record..." is more accurate than "Error ...", then the difference is whether the sentence starts with "Exception ..." vs. "IOException..." (or a subclass), so the difference is not interesting IMO. Or, we could say `"CSVParser#nextRecord(): " + e` where the (maybe dubious) benefit is that it is a touch more language-neutral.
Thoughts?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] michael-o commented on a diff in pull request #348: Error message fix
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310158527
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
Why not just `throw new UncheckedIOException("Error in reading next record", e);`? No duplication. Plain and simple.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #348: Error message fix
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #348:
URL: https://github.com/apache/commons-csv/pull/348#discussion_r1310178428
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -147,7 +147,7 @@ private CSVRecord getNextRecord() {
try {
return CSVParser.this.nextRecord();
} catch (final IOException e) {
- throw new UncheckedIOException(e.getClass().getSimpleName() + " reading next record: " + e.toString(), e);
+ throw new UncheckedIOException("Error in reading next record: " + e.toString(), e);
Review Comment:
I feel "Exception reading next record" would be more meaningful.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #348: Update exception message in CSVRecord#getNextRecord()
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #348:
URL: https://github.com/apache/commons-csv/pull/348#issuecomment-1699145061
@garydgregory : I have updated Jira issue: https://issues.apache.org/jira/browse/CSV-309 respectively.
I think we can close 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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org