Skip to content

Conversation

jbrockmendel
Copy link
Member

return finalizer(result).rename(res_name)
result = finalizer(result)

# pin name for case res_name is None and result.name is not.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is still not correct I think. As far as I know, result.name will always be None?
As mentioned in the original PR (https://github.com/pandas-dev/pandas/pull/27803/files#r311961558), I think it is here about self.name not being None.

The problem we were trying to solve is that finalize will put back the original name, which might be wrong. So that's the reason we can't pass res_name into _constructor and afterwards do finalize, as that might change the name again.

@jreback jreback added the Clean label Aug 15, 2019
@jbrockmendel
Copy link
Member Author

@jorisvandenbossche did i miss anything?

@jorisvandenbossche
Copy link
Member

Given the confusion / discussion around it, I would keep some comment explaining why you set the name after finalize (instead of passing it to _constructor). At least for me it would not be directly obvious.

Co-Authored-By: Joris Van den Bossche <jorisvandenbossche@gmail.com>
@pep8speaks
Copy link

pep8speaks commented Aug 16, 2019

Hello @jbrockmendel! 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 2019-08-19 22:49:18 UTC

@jbrockmendel
Copy link
Member Author

updated per request

@jbrockmendel
Copy link
Member Author

@jorisvandenbossche gentle ping

@jorisvandenbossche jorisvandenbossche merged commit 9f71625 into pandas-dev:master Aug 20, 2019
@jorisvandenbossche
Copy link
Member

Thanks!

@jbrockmendel jbrockmendel deleted the woops branch August 20, 2019 14:01
galuhsahid pushed a commit to galuhsahid/pandas that referenced this pull request Aug 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants