pola-rs / r-polars

Bring polars to R
https://pola-rs.github.io/r-polars/
Other
415 stars 36 forks source link

refactor!: simplify Makevars files and rename env vars for building the library #693

Closed eitsupi closed 5 months ago

eitsupi commented 5 months ago

Related to #80, #241, #300, and #445

These changes are brought over from prqlr.

eitsupi commented 5 months ago

shouldn't these be named LIB_R_POLARS_* instead?

Good point. The names are a very difficult issue, but I believe that libr_polars is the name of the actual binary library that will be generated, and adding an underscore like LIB_R_POLARS would make it confusing.

https://github.com/pola-rs/r-polars/blob/134ed5db04ae722a13203f0c4d8da5db6b5aeebc/src/Makevars.in#L6 https://github.com/pola-rs/r-polars/blob/134ed5db04ae722a13203f0c4d8da5db6b5aeebc/.github/workflows/release-lib.yaml#L26

etiennebacher commented 5 months ago

I believe that libr_polars is the name of the actual binary library that will be generated

Would it be a breaking change to rename this binary lib_r_polars? It's just weird to have libr/LIBR as prefix. Since renaming envvars is a breaking change, we might as well rename the binary at the same time.

eitsupi commented 5 months ago

Would it be a breaking change to rename this binary lib_r_polars? It's just weird to have libr/LIBR as prefix.

This name is automatically generated from the crate name "r-polars". i.e. "lib" + "r-polars" -> "libr-polars" -> "libr_polars" This is the cargo's default behavior, so I don't think there's anything strange about it.

etiennebacher commented 5 months ago

Sorry for being nitpicky on this, but I'd prefer envvars to be named R_POLARS_*, e.g R_POLARS_FEATURES. This doesn't match the binary name but from the user perspective it makes more sense IMO. The users never interact directly with the binary generated by cargo, so for them there's no consistency issue in having the binary named libr_polars and the envvars named R_POLARS_*.

eitsupi commented 5 months ago

Sorry for being nitpicky on this, but I'd prefer envvars to be named R_POLARS_*, e.g R_POLARS_FEATURES. This doesn't match the binary name but from the user perspective it makes more sense IMO. The users never interact directly with the binary generated by cargo, so for them there's no consistency issue in having the binary named libr_polars and the envvars named R_POLARS_*.

That's a good point to consider, and I was thinking the same way when I opened this PR.

However, we can separate environment variables related to Rust build from other environment variables (for example, environment variables that affect the behavior of r-polars may be added) by using "LIB" as a prefix. I thought maybe it wasn't a bad idea.

etiennebacher commented 5 months ago

However, we can separate environment variables related to Rust build from other environment variables (for example, environment variables that affect the behavior of r-polars may be added) by using "LIB" as a prefix.

Right, that's a good distinction to make, thanks for explaining.

At some point, we should add a vignette listing all possible envvars, both for the Rust build and for the use in R (in addition to what you put in the "installation" vignette).

eitsupi commented 5 months ago

Thanks for your review! I would like @sorhawell to confirm this as well, so I will merge it afterwards.

sorhawell commented 5 months ago

Thanks for your review! I would like @sorhawell to confirm this as well, so I will merge it afterwards.

Hi @eitsupi and @etiennebacher .

You probably noticed the frequency and quality of PRs and reviews have gone down lately. I think so. I will take a pause from open source for half year or more I think. I want to try out some new directions in a life. It has been a great honor to work with you. You can replace me as maintainer and/or "first author" whenever it makes sense.

I will try to commit the Rstudio completion PR and give up on the vscode completion PR.

eitsupi commented 5 months ago

@sorhawell Thanks for the reply. Glad to hear you are doing well.

I will merge this and move on.

etiennebacher commented 4 months ago

@sorhawell I hope this break will be good for you and you'll find things you enjoy doing. I'm always impressed by the amount of work you put to make this package working, thanks so much

grantmcdermott commented 4 months ago

+1 OSS work is very rewarding... but it can also be incredibly draining. @sorhawell I'm among the many grateful people for all the work that you've put into r-polars. (Equally grateful for @eitsupi and @etiennebacher picking up the maintenance reins.) Enjoy the break and I hope that it brings you a sense of rest and rejuvenation.