10000 MAINT: remove star imports · Pull Request #10254 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: remove star imports #10254

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

MAINT: remove star imports #10254

wants to merge 4 commits into from

Conversation

ghost
Copy link
@ghost ghost commented Dec 21, 2017 •
edited by ghost
Loading

This looks ugly, and that's because it is! Rather it simply exposes the ugliness that was hidden behind the star imports.

Goals for this PR:

  • Remove all star imports
  • Zero functional changes

Note: this PR will not attempt to remove duplicate imports, but it allows static analyzers to potentially solve gh-10198 with another PR.

@eric-wieser
Copy link
Member

This was done by looking at __all__?

@ghost
Copy link
Author
ghost commented Dec 21, 2017 
gh-clone numpy
cd numpy
pip install -e .
pip install python-dewildcard
dewildcard
pip install pep8radius yapf
pep8radius master --yapf --in-place

@ghost
Copy link
Author
ghost commented Dec 21, 2017

Code is here: https://github.com/xoviat/dewildcard/blob/b3e70e450ced2b591130d20a62522fa4633fe2cb/dewildcard.py

@xoviat
MAINT: Remove star imports
cb7a54a 
This commit is:
  $ pip install python-dewildcard==0.2
  $ dewildcard
@xoviat
MAINT: Reformat imports
830f99a 
This commit adjusts whitespace only
@xoviat
MAINT: Manually remove problematic globals
c387cf4 
Some globals weren't present on Python 2.7
and/or needed manual tinkering.
@xoviat
MAINT: Reformat imports
26924bb
@ghost
Copy link
Author
ghost commented Dec 22, 2017

Okay, I'm +1 on merging as-is assuming I haven't made any mistakes. The idea is to allow static analyzers to work their magic in a future PR to remove duplicates.

@charris
Copy link
Member
charris commented Dec 22, 2017

In this case I'm not convinced that changing the __init__ files improves them, they are far less readable; ugly, actually. I think our position here is that every module should have __all__ defined, and that should limit what comes in with the *.

@ghost
Copy link
Author
ghost commented Dec 22, 2017

@charris

This is a temporary step to remove the star imports, which are not recommended. It's always possible to remove the extra globals in a future PR (which I may do), but the star imports need to be removed first.

@eric-wieser
Copy link
Member

The idea is to allow static analyzers to work their magic in a future PR to remove duplicates.

I'd argue we should be trying to remove duplicates from __all__, not from the list of things we import.

@eric-wieser
Copy link
Member

Your tool is wrong here:
https://github.com/xoviat/dewildcard/blob/b3e70e450ced2b591130d20a62522fa4633fe2cb/dewildcard.py#L35

That isn't looking at __all__ to decide what * imports, which it should

@ghost
Copy link
Author
ghost commented Dec 22, 2017

@eric-wieser Thanks for the suggestions. However, I won't be working on this further until I can replace all of the star imports.

@ghost ghost closed this Dec 22, 2017
@ghost ghost deleted the remove-star-imports branch December 22, 2017 01:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @charris @xoviat
0