-
Notifications
You must be signed in to change notification settings - Fork 27
Add CSV include 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
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. |
src/Elastic.Markdown/Myst/Directives/CsvFile/CsvFileViewModel.cs
Outdated
Show resolved
Hide resolved
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. |
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.
One last nit
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.
LGTM! The max columns being a 100 seems odd to me since it will never render nicely I think. We should probably make it much lower e.g 10?
wdyt?
Yeah, it's a bit weird as a limit. Let me change that. |
This adds a CSV file directive.
Fixes #1054