-
Notifications
You must be signed in to change notification settings - Fork 67
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
Support csv import #124
base: main
Are you sure you want to change the base?
Support csv import #124
Conversation
It looks like I thought BufReader would permit partially reading a file in chunks to permit reading files larger than available RAM. It looks like the more correct way to handle that is with mmap or manually writing the code to read in chunks. I could try to switch strategies here or just make that a future enhancement, up to you. |
There is not a time constraint, if you think there is a better way, go for the better way. |
FileKind::Csv => handle_csv(&path), | ||
}?; | ||
|
||
save_to_icalc(&model, &output_path).with_context(|| "Failed to sasve file as .icalc")?; |
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.
I thought the extension was .ic
.
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.
That might be, I just kept what this file was using. Is there a standard naming convention we should ensure is used everywhere?
use std::process::{Command, ExitStatus}; | ||
|
||
#[test] | ||
fn test_simple_csv() { |
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.
There are a huge number of edge cases in CSV files that you'll want to test. Escaped (or unescaped) commas inside strings, function calls, misaligned column count, etc.
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 is a good point, however as I'm relying on the implementation we already have for copy/paste purposes, I tried to stay focused the parts relating to getting a file in rather than improving the csv parsing itself.
If it's a blocker I can add it as well when I get time to work on this, but the distinction made sense in my head.
Implement basic changes to permit CSV files to be converted to .ic
Modified paste csv function to not use a
&str
so that the data doesn't need to be entirely in memory, hopefully permitting large CSVs to be read.not addressed yet, but noticed while implementing: