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

Open discussion on renaming polars classes #149

Closed
sorhawell opened this issue Apr 20, 2023 · 10 comments · Fixed by #554
Closed

Open discussion on renaming polars classes #149

sorhawell opened this issue Apr 20, 2023 · 10 comments · Fixed by #554
Labels
enhancement New feature or request

Comments

@sorhawell
Copy link
Collaborator

sorhawell commented Apr 20, 2023

I suggest to prefix all polars classes Polars, examples would be PolarsDataFrame PolarsSeries PolarsDataType

Pros:

  • Reduces risk of namespace collision with other R packages
  • Improves readability of rust code as currently native rust-polars struct and wrapper structs are named the same:

readability example of current naming: rust-polars LazyFrame in a wrapper struct named LazyFrame. This can be a confusing to read, also the compiler error can be confusing.
image

Cons:

  • class/struct names become a bit long

What naming do you (anyone) prefer/suggest? Should we rename? Prefixed RPolars?

Bonus info

  • Camel case is the only (unless explicitly disabled) allowed naming style for structs on rust side.
  • Currently, extendr can only name R side auto generated class the same as rust side wrapper structs.
  • Since r-polars only uses 2 structs + 1 class from rust-polars to public R, whereas py-polars use 2 structs + 2 classes, the final public R side class name most be the same as the rust side wrapper struct. (**footnote)

(**the long footnote)
py-polars has 4 classes/structs from rust to python. DataFrame(rust-polars struct) -> PyDataFrame(wrapper struct) -> PyDataFrame (py03 internal class) -> DataFrame(py-polars public class). The vast majority of py-polars wrapper structs and wrapper classes are the thinnest possible.
r-polars only has 3 classes/struct from rust to python: DataFrame(rust-polars struct) -> DataFrame(wrapper struct) -> DataFrame(extendr class). The R side DataFrame class though have both a set of public methods and a private method set. An early version of r-polars had 4 classes like py-polars and was using R6. That led to more conversions needed at runtime, more boiler plate code, and class instances was no longer just an R type ExternalPtr. Also, I felt it was sometimes cumbersome to make R6 behave exactly like py-polars syntax. Instead I decided to just implement extra base R S3 generic methods for the already auto-generated extendr-classes and add public method functions.

@vincentarelbundock
Copy link
Collaborator

FWIW, I agree that this is a useful change: I can imagine DataFrame vs. data.frame being confusing for some users.

I suggest to prefix all polars classes Polars, examples would be PolarsDataFrame PolarsSeries PolarsDataType

The names above seem fine. End-users will almost never need to write those out, so the length is almost irrelevant.

@etiennebacher
Copy link
Collaborator

To clarify, does this mean that

pl$DataFrame(a = 1:5, b = 1:5)

would change to

pl$PolarsDataFrame(a = 1:5, b = 1:5)

? Or that only the methods like ncol.DataFrame() are renamed to ncol.PolarsDataFrame()?

@sorhawell
Copy link
Collaborator Author

sorhawell commented Apr 21, 2023

@etiennebacher hmm good question :/ I really just wanna have the cake and eat it to :)

long specfic class names, while short syntax. I hope it can make sense.

@eitsupi
Copy link
Collaborator

eitsupi commented Apr 21, 2023

I feel that changes should be made to some of the following.

As for the function names in R, I think it makes sense to keep DataFrame, etc. since they are basically accessed by pl$.

@sorhawell
Copy link
Collaborator Author

Rust struct names like DataFrame, to something like RDataFrame (like PyDataFrame

I agree that is ideally best, but extendr does not currently support naming the R side and rust side differently. RDataFrame might look a bit strange on R side as the public class. On rust side it might not matter too much what the name is, just that it visually different from rust-polars class names e.g. DataFrame and Series. If we one day adopt S7 or extendr proc macros are updated, we can adopt RDataFrame and RSeries on rust side.

@eitsupi
Copy link
Collaborator

eitsupi commented Apr 25, 2023

extendr does not currently support naming the R side and rust side differently.

It seems that extendr supports this. (r_name option)

#[extendr(
    r_name = "test.rename.rlike",
)]
fn test_rename() { }

https://github.com/extendr/extendr/blob/fc677380fa03c638e7913fdd96140da9ff1a5ad4/CHANGELOG.md?plain=1#L64-L75

Edit: I checked and found that r_name cannot actually be used to change the class name. I will try to update extendr.......

@eitsupi
Copy link
Collaborator

eitsupi commented Jul 1, 2023

Related to extendr/extendr#553 and #253

@eitsupi
Copy link
Collaborator

eitsupi commented Oct 16, 2023

Based on the discussion at #418, I think there is currently a consensus to adopt the prefix RPolars (e.g. RPolarsDataFrame).

@eitsupi eitsupi added this to the 1st CRAN Release milestone Oct 17, 2023
@etiennebacher
Copy link
Collaborator

Where are we on this? #418 is closed so does that mean we can now rename all the classes RPolarsDataFrame, RPolarsExpr, etc. both on Rust and R sides?

@eitsupi
Copy link
Collaborator

eitsupi commented Nov 28, 2023

Where are we on this? #418 is closed so does that mean we can now rename all the classes RPolarsDataFrame, RPolarsExpr, etc. both on Rust and R sides?

We simply need to rename the Rust structs here, since it seems unlikely that that functionality will be added to the extendr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants