-
Notifications
You must be signed in to change notification settings - Fork 38
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
minimal changes to rename package #84
Conversation
A first attempt on renaming to polars #82 |
Maybe "rename package"? |
Oups :) |
You've probably already noticed, but the |
Small news update for all the kind contributors who read PRs :) |
Congrats! (I'm going on vacation tomorrow, so might not make the cutoff. But will see if I have anything that might get in under the wire...) |
Merge branch 'main' into rename_minimal # Conflicts: # README.Rmd
Merge branch 'main' into rename_minimal # Conflicts: # README.md
Merge branch 'main' into rename_minimal # Conflicts: # README.Rmd
I notice r-universe-org/help#28 might result in duplicate packages (rpolars + polars) for some time on R-universe. But I guess we can take it as it comes. |
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.
A couple of thoughts and suggestions, mostly regarding documentation.
vignettes/polars.Rmd
Outdated
title: "Introduction to polars in R" | ||
output: rmarkdown::html_vignette | ||
vignette: > | ||
%\VignetteIndexEntry{Introduction to rpolars} | ||
%\VignetteIndexEntry{Introduction to polars in R} |
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.
Grammatically, "polars in R" sounds a bit off. Maybe change to "An Introduction to Polars from R" (or just "Polars from R").
@@ -13,7 +13,7 @@ knitr::opts_chunk$set( | |||
) | |||
``` | |||
|
|||
# rpolars | |||
# polars |
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.
Maybe this should be "Polars from R"? Although, if we're keeping with the arrow way of doing things then just "polars" is fine (see here)
@@ -26,7 +26,7 @@ The goal of this project is to bring the blazingly fast | |||
computation engine is written in Rust and this R implementation has no other | |||
dependencies than R itself (≥ 4.1.0). | |||
|
|||
Documentation can be found on the **rpolars** | |||
Documentation can be found on the **r-polars** |
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.
Perhaps personal preference but I think the link link should be to the actual index page (i.e., https://rpolars.github.io/index.html) rather than the reference manual. That way your readers are able top navigate more easily to where they want (e.g., installation section of the README/landing page or the vignette).
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 will change back to entry page. It is also much prettier now, after latest release, as the main page is now your readme contribution :)
development workflow is as follows: | ||
|
||
**Step 1:** Fork the **rpolars** repo on GitHub and then clone it locally. | ||
**Step 1:** Fork the **polars** repo on GitHub and then clone it locally. |
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.
Again, to avoid ambiguity, I would say: "..Fork the R **polars repo..."
Also: This is more to do with Step 2+ (not shown here) but is this workflow still the same even if the repo structure has changed? Aren't we using the pola-rs/rpolars/package
suggested by @eitsupi? (Or did I miss something?) If so, wouldn't they at least need to cd one directory deeper, i.e. cd r-polars/package
...
@@ -195,10 +195,10 @@ Rust toolchain | |||
|
|||
#### Development workflow | |||
|
|||
Assuming the system dependencies have been met (above), the typical **rpolars** | |||
Assuming the system dependencies have been met (above), the typical **polars** |
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.
To avoid ambiguity, I would say: "..the typical R polars workflow..."
**rpolars** package provides equivalent bindings from R. | ||
libraries available in any language. The underlying computation engine is | ||
written in Rust and can be used directly from via the polars crate. Orginally Polars bindings were implemented for Python. The **polars** R-package also provides equivalent bindings from R. Within | ||
R, python and rust each polars package is just called polars. The implemention polars in the |
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.
Couple of typos here. I would rewrite, starting with the "Within" on the previous line:
"Note that, within each language, the relevant implementation package is just called "polars". However, for clarity we may use a prefix when referring to specific implementations (i.e., rust-polars, py-polars, r-polars, nodejs-polars, 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.
Many thanks :) I add changes and Monday.
@sorhawell Your call at the end of the day, of course. But I think it's probably a good idea to merge this change sooner rather than later. (Assuming you're still happy with the renaming idea and all checks etc are passing.) I noticed that the rpolars website and repo are being shared on social media, which seems to be triggering increased traffic and contributions. (Which is great!) I personally think it's better to align the structure of the repo now, before this branch diverges too much from any new contributions. Tagging @eitsupi and @vincentarelbundock in case they have a different opinion. |
Thanks for the tag. I don't really have a view on this. |
I too think this should be merged. From extendr's Discord conversation, @sorhawell seems to be busy these days. This is a breaking change and should be noted in NEWS, etc., but I don't think it needs to be included in this PR since that can be done later. |
Ok, let's merge. |
commit all, a few tests fail in check only, not sure why