-
Notifications
You must be signed in to change notification settings - Fork 26
Add CSV file directive #1742
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
base: main
Are you sure you want to change the base?
Add CSV file directive #1742
Conversation
🔍 Preview links for changed docs |
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.
Nice!
Can we defer the parsing to when we actually need it?
This to keep the data on AST itself smaller.
We can leverage this csv library for low overhead reading the data out of the csv. Should help reduce allocations. E.g we could read the CSV inside the template (cshtml) and write the data to its output.
@Mpdreamz I think this is it (as for using only Sep). |
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.
ReadCsv should be an IEnumerable so that we can read and discard data through exposing the reader from Sep directly.
That way if someone does use a 2gb CSV file it won't need to be loaded in memory all at once.
That said we should have some sane defaults and validation on the max size of the csv files wdy reckon @theletterf
the csv I need to render is a somewhat biggie at ~250k so we should set a relatively large max |
@Mpdreamz @shainaraskas Changes made, perhaps we could test it with a humongous CSV? New params:
|
tested it out and the 3 params work as expected. tried with a big csv (20k rows) and it worked fine. I tested it with a 22 column, 2748 row spreadsheet and it was ok too. |
@theletterf appologies should have been clearer on my end. I think it's best to hard code these parameters and not make them configurable. E.g these should be validations against abuse/unintended usages not open for configuration as options. |
|
||
public class CsvFileViewModel : DirectiveViewModel | ||
{ | ||
public required List<string[]> CsvRows { get; init; } |
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 should also be IEnumerable<string[]>
, probably easier to make it a method.
That way the cshtml can lazily iterate over large csv files too.
Let me know if you want me to pick that up in a follow up @theletterf !
Does this close #1054? |
Co-authored-by: shainaraskas <[email protected]>
@Mpdreamz @shainaraskas Done in 40c24f4:
|
20000 rows ✅ my only note: what benefit does preview mode bring, given that we don't have a download option? |
@shainaraskas You're right! Preview mode removed. I also increased the row limit to 25k, since your test would have been truncated otherwise (I wonder though, do we really plan on showing such a long CSV file in the docs?) |
@theletterf I was just testing that the limits were working. my use case is a file under 3k lines. |
This adds a CSV file directive.
Fixes #1054