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

std/encoding/csv.ts - parse and stringify should not require async code when data is already in memory #880

Closed
dsherret opened this issue Apr 26, 2021 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@dsherret
Copy link
Member

Right now, the code in https://github.com/denoland/deno/blob/std/0.82.0/std/encoding/csv.ts forces the caller of parse and stringify to await the result, but there is no reason for these functions to be async when the object is already in memory.

@lowlighter
Copy link
Contributor

lowlighter commented Apr 27, 2021

All async code requirement seems to come from this single line:
https://github.com/denoland/deno_std/blob/3931030e7fae16158e3b6cb041a2a161c0cba6e3/encoding/csv.ts#L262

And from the function signature, it seems both string and BufReader are supported:
https://github.com/denoland/deno_std/blob/3931030e7fae16158e3b6cb041a2a161c0cba6e3/encoding/csv.ts#L392-L393

Not sure why we actually support BufReader (maybe because it was ported from Go?) but I think we should drop the support for it. With only strings as input it would be easier to make it synchronous.

YAML/TOML (and native JSON) libs only support strings as input so it would actually be more consistent

PS: seems you linked an old version of std (0.82.0 v 0.95.0) though it's still revelant as of today

@bartlomieju bartlomieju added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Nov 10, 2021
@MierenManz
Copy link
Contributor

I want to pick up this issue. It seems like I only have the remove the BufReader input and the code related to that? I hope I understood that correctly tho

@MierenManz
Copy link
Contributor

Anyone else can try if they want.
I don't got the time to pick up this issue.

@kt3k
Copy link
Member

kt3k commented Nov 24, 2021

(Sorry for the late reply, but) I think async parse (with BufReader) might make sense if the source csv file is very big. That should reduce the memory usage and execution time.

@luk3skyw4lker
Copy link
Contributor

luk3skyw4lker commented Apr 3, 2022

I could take a look on that too, I'm already on the works of another issue envolving the same file. I'm just not sure if this is already solved.

@lino-levan
Copy link
Contributor

@kt3k This issue should probably be closed. std/encoding/csv.ts is no longer async and it also no longer supports BufReader.

Sanity check: https://deno.land/std@0.168.0/encoding/csv.ts?doc=&s=parse

@kt3k
Copy link
Member

kt3k commented Dec 19, 2022

@lino-levan Thanks for the ping. Right, both are now sync. This issue should be now closed.

refs:

@kt3k kt3k closed this as completed Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants