-
Notifications
You must be signed in to change notification settings - Fork 621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(csv): remove dead code #5269
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5269 +/- ##
==========================================
+ Coverage 95.70% 95.74% +0.03%
==========================================
Files 459 459
Lines 38025 38012 -13
Branches 5563 5562 -1
==========================================
+ Hits 36393 36394 +1
+ Misses 1591 1577 -14
Partials 41 41 ☔ View full report in Codecov by Sentry. |
} else { | ||
// Abrupt end of file (EOF on error). | ||
if (!opt.lazyQuotes) { | ||
const col = runeCount(fullLine); | ||
quoteError = new ParseError( | ||
startLine + 1, | ||
lineIndex, | ||
col, | ||
ERR_QUOTE, | ||
); | ||
break parseField; | ||
} | ||
fieldIndexes.push(recordBuffer.length); | ||
break parseField; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is unreachable, as line.length === 0
and line.length > 0
are both involved in logical OR expressions above. It's impossible for line.length < 0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't confirm your reasoning. This else-block seems a part of the below structure. Is line.length === 0
involved in these conditions?
if (i >= 0) {
// ...
} else if (line.length > 0 || !reader.isEOF()) {
// ...
} else {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line
variable is modified in the below part. https://github.com/denoland/deno_std/blob/b968c2fe21063030f972d71f846930cb9a98ceb8/csv/_io.ts#L119
I don't think that reasoning works if line
is modified in the middle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe #5277 might help for more clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll close this until CSV simplification changes are made.
Brings
@std/csv
coverage to 100%.Towards #3713