-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(python): allow df.rename and lf.rename to take a renaming function #13708
Conversation
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.
Nice - I think the idea makes sense, and the use of pl.all().name.map
looks clean; good stuff! A few minor comments, and then can you add some unit tests? Would help to see it in action to validate behaviour. Once the nits are dealt with and there is some code coverage I'll approve 👍
@alexander-beedie just added a couple of tests. Did you mean to leave comments on the code as well? |
@alexander-beedie how's this looking, would you say it's ready to merge? |
I don't see that you've addressed the inline comments yet? |
@alexander-beedie you'll have to forgive me if I'm being thick, but I don't see any comments. Usually they'd appear in the conversation tab, but here's what the conversation tab looks like for me: and here's the files changed tab: |
@Wainberg I don't think you're being thick, I can't see them either. @alexander-beedie Could you re-post those comments? |
Ouch; I blame the GitHub UI for that one; I could see them marked as "Pending", which I thought was "pending action by the author of the PR", but apparently means "Pending you hitting the 'submit' button so the author can see them", sigh... Apologies for that one @Wainberg; think they should be visible now 🤣 |
@alexander-beedie No worries! Just made the two changes you suggested. |
Looks good to me 👌 |
…on (pola-rs#13708) Co-authored-by: Wainberg <m.wainberg@utoronto.ca>
Closes #13229.
There seemed to be a lot of popular demand for this (based on the number of likes on the original issue), so I went ahead and implemented it.