docs(csv): more examples for parse and CsvParseStream
#5605
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5605 +/- ##
=======================================
Coverage 96.37% 96.37%
=======================================
Files 466 466
Lines 37572 37574 +2
Branches 5539 5539
=======================================
+ Hits 36211 36213 +2
Misses 1319 1319
Partials 42 42 ☔ View full report in Codecov by Sentry. |
| * const result = parse(string, { skipFirstRow: true }); | ||
| * | ||
| * assertEquals(result, [{ a: "d", b: "e", c: "f" }]); | ||
| * assertType<IsExact<typeof result, Record<string, string | undefined>[]>>(true); |
There was a problem hiding this comment.
I added assertType here because I think this is a very good document for users given that the return type varies depending on the option we pass in.
While I was writing these assertions, though, I was unsure in what cases the value in the return value becomes undefined. Does anyone know why we have Record<string, string | undefined> instead of Record<string, string> here?
There was a problem hiding this comment.
I think you can get an undefined value if you had the following input a,b,c\nd,,f. Perhaps, we should add another test for that case.
There was a problem hiding this comment.
I tried the following three test cases and these pass. I couldn't find a case where the parsed value becomes undefined
await t.step({
name: "BlankField2",
fn() {
const input = "a,b,c\nd,,f";
assertEquals(
parse(input, { skipFirstRow: true }),
[{ a: "d", b: "", c: "f" }],
);
},
});
await t.step({
name: "Exessive fields with skipFirstRow: true",
fn() {
const input = "a,b,c\nd,,f,g";
assertThrows(
() => parse(input, { skipFirstRow: true }),
Error,
"Error number of fields line: 1\nNumber of fields found: 3\nExpected number of fields: 4",
);
},
});
await t.step({
name: "Insufficient fields with skipFirstRow: true",
fn() {
const input = "a,b,c\nd,e";
assertThrows(
() => parse(input, { skipFirstRow: true }),
Error,
"Error number of fields line: 1\nNumber of fields found: 3\nExpected number of fields: 2",
);
},
});There was a problem hiding this comment.
Added (or fixed the existing) tests on what happens if the number of fields in records doesn't match that of the header. This fix contains the change in how we display the line number is error messages (which was 0-based but now changed to 1-based).
| * const result = parse(string, { skipFirstRow: true }); | ||
| * | ||
| * assertEquals(result, [{ a: "d", b: "e", c: "f" }]); | ||
| * assertType<IsExact<typeof result, Record<string, string | undefined>[]>>(true); |
There was a problem hiding this comment.
I think you can get an undefined value if you had the following input a,b,c\nd,,f. Perhaps, we should add another test for that case.
| * assertEquals(result, [["a", "b", "c"], ["d", "e", "f"]]); | ||
| * ``` | ||
| * | ||
| * @example Trim leading space with `trimLeadingSpace: true` |
There was a problem hiding this comment.
Is there a reason there's an option to trim only leading spaces? Why not just trim the end too using String.prototype.trim()?
There was a problem hiding this comment.
I'm not sure either, but according to the head comment of parse.ts, many parts of the implementation was ported from Go which has the same option.
https://github.com/golang/go/blob/go1.12.5/src/encoding/csv/reader.go#L136
There was a problem hiding this comment.
I can't think of a reason to only trim the start... I suggest changing to trim: boolean. Not a strong opinion. What do we think?
There was a problem hiding this comment.
Asked ChatGPT, it answered some application may add leading whitespaces when exporting to CSV. I don't know if this is true at all 😄 Anyway trim: boolean should be useful in some scenarios and I think we could add that after 1.0 without breaking things?
There was a problem hiding this comment.
I think trailing spaces are usually considered as a part of value of the last column (For example, excel handles it in that way)
There was a problem hiding this comment.
I couldn't find the case when this returns Record<string, string | undefined>, but I also noticed that fieldsPerRecord option doesn't seem working as described.
It says:
If negative, no check is made and records may have a variable number of fields.
But parse doesn't seem allowing wrong number of fields even when I passed negative number to this option. Maybe this current state is related to it?
There was a problem hiding this comment.
But parse doesn't seem allowing wrong number of fields even when I passed negative number to this option. Maybe this current state is related to it?
Oh that's a good find. I'll add a test case and fix it
…#5617) As we discussed in #5605 (comment), it seems like we never get `undefined` as a parse result of fields. If there is a mismatch in the number of fields across rows, the parse just throws an error. To better reflect this in typing, this commit removes `undefined` from the record value type.
This commit adds more examples to
parsefunction andCsvParseStreamclass to cover all the provided options.Also fixes a few other things:
ParseErrorwith the correctSyntaxError.commentproperty. The old comment says the default value is#, but this was wrong.fieldsPerRecordoption working inparseas documented (closes csv:fieldsPerRecord: -1doesn't seem working #5616)