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

STY: Changed x.__class__ to type(x) #29816

Closed
wants to merge 3 commits into from

Conversation

ShaharNaveh
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@ShaharNaveh ShaharNaveh marked this pull request as ready for review November 23, 2019 18:59
@WillAyd
Copy link
Member

WillAyd commented Nov 23, 2019

Is this supposed to make any difference in Python3 or just a stylistic thing? If so, lgtm

@WillAyd WillAyd added the Code Style Code style, linting, code_checks label Nov 23, 2019
@WillAyd WillAyd added this to the 1.0 milestone Nov 23, 2019
@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Nov 23, 2019

Is this supposed to make any difference in Python3 or just a stylistic thing? If so, lgtm

Pure stylistic change. trying to be consistent.

something that was mentioned here.

@jbrockmendel
Copy link
Member

Two questions, not blockers

  • Is there a meaningful difference between using type(self) vs type(self).__name__?
  • Can/should we implement a code_check for this?

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Nov 25, 2019

* Is there a meaningful difference between using `type(self)` vs `type(self).__name__`?

The Python docs explains this much better.

In 2 lines summary it goes like this:

  • type(self) --> '<class self>' (type type)
  • type(self).__name__ --> 'self' (str type)

The detailed explanation bellow:

The way I see it if you're comparing only type(self) and type(self).__name__, type(self) will return a type class, if you print the output as a string, you'll get <class self>. for example:

>>> my_str = 'Example'
>>> type(my_str)
<class 'str'>
>>> type(type(my_str))
<class 'type'>

But if you add .__name__ to it, it will return a str class, and if you print it as a string it will print only the class name (as a string).
For example:

>>> my_int = 1
>>> type(my_int).__name__
'int'
>>> type(type(my_int).__name__)
<class 'str'>

P.S
Really hope my explanation didn't confuse anyone.

@jbrockmendel
Copy link
Member

Mostly more confusing; I think the focus needs to be on str(...) and not type(...). It does seem clear though that using __name__ should be preferred, yah?

@ShaharNaveh
Copy link
Member Author

@jbrockmendel Now I'm confused lol.

type(self) and type(self).__name__ returns totally different things.

Can you give an example(preferably code) of what is preferred and what should not be?

Would love to correct my PR accordingly.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. can you implement a code_check for the now banned format as well? (basically grep for self.__class__ i think would be enough

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Nov 26, 2019

lgtm. can you implement a code_check for the now banned format as well? (basically grep for self.__class__ i think would be enough

@jreback TBH no, I have no clue on to approach that (writing tests).

Plus I don't think a simple grep will do the work unfortunately. there are files like pandas/core/indexing.py with that self.__class__ syntax

new_self = self.__class__(self.name, self.obj)

But I do think that if we ban the self.__class__.__name__ we should be alright.

@jreback
Copy link
Contributor

jreback commented Nov 26, 2019

why is this ok? self.__class__(self.name, self.obj)

that should simply be type(self)(self.name, self.obj) no?

@ShaharNaveh
Copy link
Member Author

why is this ok? self.__class__(self.name, self.obj)

that should simply be ```type(self)(self.name, self.obj)`` no?

Gave me an error when I tried exactly what you wrote.
But please do check it yourself. I might screwed something elsewhere that has nothing to do with that error.

@jbrockmendel
Copy link
Member

@MomIsBestFriend pls ignore my previous comment(s), it looks like they only made things more confusing without being productive. This PR was/is fine before I started talking.

@jreback let's hold off on a code check for the next round; the diff here is already wide-reaching.

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Nov 26, 2019

@jreback looks like new_self = type(self)(self.name, self.obj) is indeed the correct syntax replacement for new_self = self.__class__(self.name, self.obj) (I don't know why I got an error before).
Ill push the different version later.

@ShaharNaveh ShaharNaveh changed the title CLN: Changed x.__class__ to type(x) STY: Changed x.__class__ to type(x) Nov 26, 2019
@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Nov 27, 2019

Since this PR got too large, and I can't detect where is the error, I'm closing it.
I will push this PR in batches instead of one large PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Style Code style, linting, code_checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants