Skip to content

CLN: Use ABC classes for isinstance checks, remove unnecessary imports #28158

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 2 commits into from
Aug 27, 2019

Conversation

addisonlynch
Copy link
Contributor

Per #27353 changed isinstance checks listed in the @WillAyd script to use ABCs where it enabled removal of imports only.

Note there may be additional files which can be cleaned as the script does not parse multi-line statements.

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

@WillAyd
Copy link
Member

WillAyd commented Aug 26, 2019

Nice PR! Any idea for how many more there might be?

@WillAyd WillAyd added the Clean label Aug 26, 2019
@addisonlynch
Copy link
Contributor Author

There are a large number of additional isinstance checks which could be changed (40-50 in the files that came up in your script), but I'm not sure how many of them will allow removal of the child imports -- thus won't update those per @jbrockmendel's benchmark.

I'll keep digging for more which do allow import removals...do you want me to add any additional to this PR or open separate? May not get it all done at once.

@WillAyd
Copy link
Member

WillAyd commented Aug 26, 2019

I'm OK with doing it incrementally in separate PRs. Just ultimately want to make sure we have an end in sight. However you feel best to manage that works!

@WillAyd WillAyd added this to the 1.0 milestone Aug 26, 2019
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Lgtm

@WillAyd
Copy link
Member

WillAyd commented Aug 27, 2019

@jbrockmendel thoughts here? Merge if happy

@jbrockmendel
Copy link
Member

Thanks @addisonlynch

@jbrockmendel jbrockmendel merged commit 080d57e into pandas-dev:master Aug 27, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
pandas-dev#28158)

* CLN: Use ABC classes for isinstance checks, remove unnecessary imports

* Formatting repairs
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
pandas-dev#28158)

* CLN: Use ABC classes for isinstance checks, remove unnecessary imports

* Formatting repairs
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.

3 participants