Skip to content

Deprecate mapper argument in rename #44672

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

Merged
merged 8 commits into from
Dec 5, 2021
Merged

Conversation

Varun270
Copy link
Contributor

Comment on lines 4545 to 4546
mapper = index
if callable(mapper) or is_dict_like(mapper):
Copy link
Member

Choose a reason for hiding this comment

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

can also revert these lines (and the 2 above, and the call involving mapper below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I am familiar with the fact that I haven't reverted all the changes of series.rename just wanted to run it by you so that you can confirm whether I am on the right track?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will force push the rest of the changes if you give me a go-ahead.

Copy link
Member

Choose a reason for hiding this comment

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

yes, go ahead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the rest of the changes, review them whenever you are free!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function test_rename_no_method_no_index no longer exists. So should I remove the function test_rename_method_and_index ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes you can remove test_rename_method_and_index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Entire function?

Copy link
Member

Choose a reason for hiding this comment

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

yup, if we're removing mapper it's no longer relevant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it.

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

CI checks are failing, can you address them please?

pandas/core/generic.py:4421: error: unused "type: ignore" comment
pandas/core/generic.py:4485: error: unused "type: ignore" comment

@Varun270
Copy link
Contributor Author

CI checks are failing, can you address them please?

pandas/core/generic.py:4421: error: unused "type: ignore" comment
pandas/core/generic.py:4485: error: unused "type: ignore" comment

Sorry, I don't know how to address failing checks, I don't know anything about CI/CD.

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Nov 30, 2021

you can just remove those two # type: ignore comments

I'd suggest you read through the contributing guide in full too https://pandas.pydata.org/docs/development/contributing.html

@Varun270
Copy link
Contributor Author

you can just remove those two # type: ignore comments

I'd suggest you read through the contributing guide in full too https://pandas.pydata.org/docs/development/contributing.html

Yeah sure, Thanks for your effort.

@pep8speaks
Copy link

pep8speaks commented Nov 30, 2021

Hello @Varun270! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-02 14:57:56 UTC

@Varun270
Copy link
Contributor Author

Varun270 commented Dec 2, 2021

you can just remove those two # type: ignore comments

I'd suggest you read through the contributing guide in full too https://pandas.pydata.org/docs/development/contributing.html

You can review it again I have done the changes that you suggested.

@@ -4418,7 +4418,7 @@ def add_prefix(self: NDFrameT, prefix: str) -> NDFrameT:
# expected "NDFrameT")
# error: Argument 1 to "rename" of "NDFrame" has incompatible type
# "**Dict[str, partial[str]]"; expected "Union[str, int, None]"
return self.rename(**mapper) # type: ignore[return-value, arg-type]
return self.rename(**mapper)
Copy link
Member

@MarcoGorelli MarcoGorelli Dec 2, 2021

Choose a reason for hiding this comment

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

ah, this needs to be self._rename (and then you'll likely need to put the type: ignore back - sorry, I hadn't spotted this earlier)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah no problem will do it

@@ -4482,7 +4482,7 @@ def add_suffix(self: NDFrameT, suffix: str) -> NDFrameT:
# expected "NDFrameT")
# error: Argument 1 to "rename" of "NDFrame" has incompatible type
# "**Dict[str, partial[str]]"; expected "Union[str, int, None]"
return self.rename(**mapper) # type: ignore[return-value, arg-type]
return self.rename(**mapper)
Copy link
Member

Choose a reason for hiding this comment

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

likewise (self._rename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed the changes, You can review them whenever you are free.

Copy link
Member

Choose a reason for hiding this comment

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

this'll fail CI, can you run the black formatter on the files you modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't installed black also I don't know much about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have installed it and trying to figure out how it works. Is this document good to refer to https://www.freecodecamp.org/news/auto-format-your-python-code-with-black/

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks @Varun270

@Varun270
Copy link
Contributor Author

Varun270 commented Dec 2, 2021

Thanks @Varun270

Thanks for helping me, I learned a lot!!

@jreback jreback added this to the 1.4 milestone Dec 5, 2021
@jreback jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Dec 5, 2021
@jreback jreback merged commit ed5a004 into pandas-dev:master Dec 5, 2021
@jreback
Copy link
Contributor

jreback commented Dec 5, 2021

thanks @Varun270

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DEPR Deprecate mapper argument in rename?
4 participants