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

refactor!: Rename structs in Rust to rename classes in R #554

Merged
merged 35 commits into from
Dec 1, 2023

Conversation

etiennebacher
Copy link
Collaborator

Close #149

I just renamed the LazyFrame class (on R side) and struct (on rust side) to RPolarsLazyFrame. This compiles and works fine. We don't need to change the docs since this is only an internal (breaking) change.

@sorhawell @eitsupi if you agree with this then I can move forward and rename DataFrame and Expr

@etiennebacher etiennebacher marked this pull request as draft November 29, 2023 08:31
@sorhawell

This comment was marked as duplicate.

@sorhawell
Copy link
Collaborator

This looks very good! I would see it as minor change as some user may have written using class() or inherits() and there have to adjust the class name.

Funny thing @paleolimbot suggested this change very early in the project history, and I was like no that would be terrible. I finally came around to like it :)

If you like you can also split it into a couple of PRs.

sorhawell
sorhawell previously approved these changes Nov 29, 2023
@sorhawell sorhawell self-requested a review November 29, 2023 08:47
@sorhawell sorhawell dismissed their stale review November 29, 2023 08:48

I looked to as if the checks were passing. I they are not and this is a draft.

@etiennebacher
Copy link
Collaborator Author

I would see it as minor change as some user may have written using class() or inherits() and there have to adjust the class name.

Yes I'll put this in the NEWS

If you like you can also split it into a couple of PRs.

I think I'll do all at once here

@eitsupi
Copy link
Collaborator

eitsupi commented Nov 29, 2023

Thanks for working at this!
I'm definitely in favor of making all class name changes.

@etiennebacher
Copy link
Collaborator Author

I'm wondering if I should add the RPolars prefix to the other classes too, like LazyGroupBy or DataType:

https://github.com/pola-rs/r-polars/pull/554/files#diff-afa4237c90018088718e50ed6bf29c5d33bcf94ba925a9a5089683ba258e1343R270-R272

What do you think?

@eitsupi
Copy link
Collaborator

eitsupi commented Nov 29, 2023

I'm wondering if I should add the RPolars prefix to the other classes too

I think so.

src/rust/Cargo.toml Outdated Show resolved Hide resolved
@etiennebacher etiennebacher marked this pull request as ready for review November 30, 2023 16:17
@eitsupi eitsupi changed the title Rename classes refactor!: Rename structs in Rust to rename classes in R Dec 1, 2023
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.

Thanks! Awesome work!

@eitsupi eitsupi force-pushed the rename-classes branch 2 times, most recently from aa5d9ec to 1fd4785 Compare December 1, 2023 11:26
@eitsupi
Copy link
Collaborator

eitsupi commented Dec 1, 2023

I think we may have to merge #516 first.

@sorhawell
Copy link
Collaborator

I think we may have to merge #516 first.

I have updated #516 , ready for review again

@etiennebacher
Copy link
Collaborator Author

Thanks for completing this PR @eitsupi

@etiennebacher etiennebacher marked this pull request as ready for review December 1, 2023 17:15
@etiennebacher etiennebacher merged commit 396caf9 into main Dec 1, 2023
30 checks passed
@etiennebacher etiennebacher deleted the rename-classes branch December 1, 2023 17:54
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.

Open discussion on renaming polars classes
3 participants