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

Implement $write_csv() for DataFrame #414

Merged
merged 30 commits into from
Oct 18, 2023
Merged

Implement $write_csv() for DataFrame #414

merged 30 commits into from
Oct 18, 2023

Conversation

etiennebacher
Copy link
Collaborator

TODO: more checks for several parameters (datetime_format, etc.)

tests/testthat/test-csv.R Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator

eitsupi commented Oct 16, 2023

It seems that there is an error with non_numeric.

> 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

R/dataframe__frame.R Outdated Show resolved Hide resolved
R/dataframe__frame.R Outdated Show resolved Hide resolved
@etiennebacher
Copy link
Collaborator Author

It seems that there is an error with non_numeric.

has_header is the 2nd arg and takes a boolean, hence the error. Should it be handled differently?

@etiennebacher
Copy link
Collaborator Author

etiennebacher commented Oct 17, 2023

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

@etiennebacher etiennebacher marked this pull request as ready for review October 17, 2023 11:33
@sorhawell
Copy link
Collaborator

sorhawell commented Oct 17, 2023

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 ?- operator as the user is inherently fallible.

? operator is kinda short hand for

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 let f = std::fs::File::create(path)?; where any std::fs::File error is implicitly converted RPolarsError.

>   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 fallible_foo() -> RResult<Bar>cannot forget to address the Result has two possible states. When prototyping it is fine to use unwrap(), but should be replaced with something else. The rust value based error system becomes very powerful when dealing with nested error scenarios. Anyways for 95% of our code we can ?

@etiennebacher
Copy link
Collaborator Author

Thanks for improving the code @sorhawell

Copy link
Collaborator

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@eitsupi eitsupi merged commit d6ab1da into main Oct 18, 2023
11 checks passed
@eitsupi eitsupi deleted the write_csv branch October 18, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants