-
Notifications
You must be signed in to change notification settings - Fork 36
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
Implement $write_csv()
for DataFrame
#414
Conversation
It seems that there is an error with > pl$DataFrame(a = 1)$write_csv("tmp.csv", "non_numeric")
thread '<unnamed>' panicked at src/rdataframe/mod.rs:469:52:
called `Result::unwrap()` on an `Err` value: RPolarsErr { contexts: [BadVal("Rvalue: [\"non_numeric\"], Rsexp: Strings, Rclass: [\"character\"]"), Mistyped("bool"), BadArg("has_header")], rcall: None, rinfo: None }
thread '<unnamed>' panicked at src/rdataframe/mod.rs:83:1:
explicit panic
Error in .pr$DataFrame$write_csv(self, path, has_header, utf8ToInt(separator), :
user function panicked: write_csv |
|
I added some tests for remaining options but some of your comments are still unclear to me. I mark this as ready for review and feel free to push directly on this branch |
in rust unwrap says "surely this could never be None/Err" and leads to unrecoverable panic if ever wrong. The ? also accessed the Some/Ok value OR "bubble" the error very nicely upwards. When parsing user input we should always prefer
if x.is_err() {
return( x.into())
} else {
x.unwrap_unchecked()
} the .into() allows for some errors to be implicitly converted. We do use that at > ctx = dat_pl2$write_csv("/", quote = "a") |> get_err_ctx()
> ctx
$PlainErrorMessage
[1] "Os { code: 21, kind: IsADirectory, message: \"Is a directory\" }" If we were to unwrap a parsing-error, the panic stack traces are forwarded to the StdError prompt, not the R prompt. Extendr is getting closer, but I would not say panic! is completely a stable feature in terms of prompt and memory management. Depending on R IDE, we can still read the panic message if it redirects the StdError prompt. In rust if we really believe some thing should never go further on, we can use panic! and unreachable! if we believe it could never come to that. unwrap is kinda cowboy coding / placeholder coding and should at least be replaced with .expect("me the dev, says this would never be a None/Err because I know that ...") where we state our run time assertion. In development I use todo!() alot because it allows me to not complete a function without rustanalyzer screams at me. I struggle writing rust without rustanalyzer telling me interactively what types I got. I do use unwrap() temporarily if I'm unsure about some error conversion, and that is fair enough. When we don't know if something could be an Err we have to model this with the rust error type system. Any one who calls |
Merge remote-tracking branch 'origin/main' into write_csv # Conflicts: # NEWS.md
Thanks for improving the code @sorhawell |
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.
Looks good, thanks!
TODO: more checks for several parameters (
datetime_format
, etc.)