Skip to content
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

csv - parse takes undefined opt #4545

Closed
dsherret opened this issue Apr 3, 2024 · 7 comments · Fixed by #4920
Closed

csv - parse takes undefined opt #4545

dsherret opened this issue Apr 3, 2024 · 7 comments · Fixed by #4920
Labels
bug Something isn't working csv good first issue Good for newcomers PR welcome A pull request for this issue would be welcome

Comments

@dsherret
Copy link
Member

dsherret commented Apr 3, 2024

Kind of confusing that the opt parameter here is opt?: undefined:

https://github.com/denoland/deno_std/blob/74fc8ff920991c4c79fa4c7647d855e5780db64c/csv/parse.ts#L326

...shouldn't that just not exist given the other overloads?

@dsherret dsherret added bug Something isn't working needs triage labels Apr 3, 2024
@iuioiua iuioiua added good first issue Good for newcomers csv PR welcome A pull request for this issue would be welcome and removed needs triage labels Apr 8, 2024
@fdemir
Copy link

fdemir commented Apr 14, 2024

Should we remove the opt?: undefined from this overload?

@dsherret
Copy link
Member Author

Yeah, I think it should just be export function parse(input: string): string[][];

@Wencho8
Copy link
Contributor

Wencho8 commented Apr 16, 2024

I can work on this. Removing the extra comments and the opt parameter.

Wencho8 added a commit to Wencho8/deno_std that referenced this issue Apr 16, 2024
iuioiua pushed a commit that referenced this issue Apr 16, 2024
Remove unused opt parameter (issue #4545), since the other overloads already handle it.
@iuioiua
Copy link
Contributor

iuioiua commented Apr 16, 2024

Completed in #4598

@iuioiua iuioiua closed this as completed Apr 16, 2024
@dandv
Copy link

dandv commented May 25, 2024

Not sure what the status of this is, but I'm giving Deno a first try with a simple task: manipulate a CSV. Trying to figure out the parameters for parse in WebStorm is tricky, because the options object doesn't seem to be documented (only input is), but is used in the example:

image

I'm using jsr:@std/csv@^0.224.1.

@BlackAsLight
Copy link
Contributor

Not sure what the status of this is, but I'm giving Deno a first try with a simple task: manipulate a CSV. Trying to figure out the parameters for parse in WebStorm is tricky, because the options object doesn't seem to be documented (only input is), but is used in the example:

image

I'm using jsr:@std/csv@^0.224.1.

Looking at the documentation, parse has two overloads, with opt being a generic type that extends ParseOptions. Why opt isn't a Partial<ParseOptions> is unknown to me.

@iuioiua
Copy link
Contributor

iuioiua commented May 29, 2024

Re-opening this issue. It's weird that parse() is generic and could be cleaned up and documented better.

Edit: I'll keep this open until CSV documentation is reviewed as part of achieving stabilization. Then, we can see if the documentation satisfies the shortcomings pointed out here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working csv good first issue Good for newcomers PR welcome A pull request for this issue would be welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants