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

Implement pandas like rename #1135

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

janakact
Copy link

Intended to fix #80

@maartenbreddels
Copy link
Member

Hi,

thanks for your first-time contribution! I like the idea of this, but I'm not sure of the method name. @JovanVeljanoski what do you think. Should we mayne rename the existing rename, or have 1 rename method that takes a dict or two arguments?

Regards,

Maarten

@janakact
Copy link
Author

janakact commented Feb 2, 2021

Hi @maartenbreddels, Thank you for this marvelous library.
Yeah. Of course name should be changed I think, it should be similar to pandas I guess. May I add an if condition to check whether the input is a dict to support both vaex and pandas API?

Copy link
Member

@JovanVeljanoski JovanVeljanoski left a comment

Choose a reason for hiding this comment

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

I wonder if the current rename and pandas_rename can be refactored to be a single function.

For example, if the name argument is collections.Mapping instance, than do what pandas_mapping does, otherwise assume the current rename method's functionality.

Also, would be nice if this branch were rebased on master, lots of changes in the meantime, would help with the CI.

"""Rename columns like pandas. Mappings should contain a dict like object"""
if not isinstance(mappings, collections.Mapping):
raise TypeError('Mappings object is not a dict like object')
df = self.copy()
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to do df = self.copy() instead of just doing the rename on self directly (like the existing rename method is doing? (Maybe @maartenbreddels knows).

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.

rename_column to excepts dict
3 participants