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

feat!: make $rename()'s behavior closer to python polars' #1129

Merged
merged 6 commits into from
Jun 4, 2024

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Jun 3, 2024

A follow up for #1122

  • old_name = "new_name" instead of new_name = "old_name"
  • Allow a function input

@eitsupi eitsupi requested a review from etiennebacher June 3, 2024 13:47
@eitsupi eitsupi mentioned this pull request Jun 3, 2024
@eitsupi eitsupi changed the title feat!: $rename() with a function instead of $rename_with() feat!: make $rename()'s behavior closer to python polars' Jun 3, 2024
@eitsupi eitsupi marked this pull request as ready for review June 3, 2024 14:07
@eitsupi
Copy link
Collaborator Author

eitsupi commented Jun 3, 2024

@etiennebacher This will definitely break tidypolars, but hopefully it is a meaningful change in terms of consistency with Python.

@eitsupi eitsupi added this to the 0.17 milestone Jun 3, 2024
Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

Nice, I prefer this order and actually I originally put this old = new order when I implemented it one year ago but we decided to follow dplyr for more consistency (#239).

This will definitely break tidypolars

Not a big deal, I'll just need to update the internals but it won't change the external behaviour.

Also for those who use polars directly, this will clearly error and not just fail silently.

R/lazyframe__lazy.R Outdated Show resolved Hide resolved
R/lazyframe__lazy.R Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator Author

eitsupi commented Jun 3, 2024

Nice, I prefer this order and actually I originally put this old = new order when I implemented it one year ago but we decided to follow dplyr for more consistency (#239).

Hmmm, I completely forgot about this.
And I always have trouble with the order of dplyr::rename() as well (although new=old makes more sense since it is consistent with dplyr::select())

@eitsupi eitsupi force-pushed the rename-breaking branch from b63a033 to d477c2b Compare June 3, 2024 23:18
@eitsupi eitsupi merged commit 52cb990 into main Jun 4, 2024
18 of 20 checks passed
@eitsupi eitsupi deleted the rename-breaking branch June 4, 2024 03:01
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.

2 participants