8000 MAINT: removed unused imports listed in LGTM by ajayd-san · Pull Request #19090 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: removed unused imports listed in LGTM #19090

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
Merged
merged 2 commits into from
May 25, 2021
Merged

MAINT: removed unused imports listed in LGTM #19090

merged 2 commits into from
May 25, 2021

Conversation

ajayd-san
Copy link
Contributor

Removed most of the unused imports mentions in LGTM.

closes #19077

eric-wieser
eric-wieser reviewed May 25, 2021
View reviewed changes
Copy link
Member
@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Not all unused imports are safe to remove; some of them have side-effects, while others are part of the public API

numpy/compat/py3k.py Outdated
@@ -21,9 +21,6 @@
from pathlib import Path
import io

import abc
from abc import ABC as abc_ABC
Copy link
Member

Choose a reason for hiding this comment

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

The fact these are imported here is considered public API for this module

numpy/lib/function_base.py Outdated
@@ -33,7 +33,6 @@
import builtins

# needed in this module for compatibility
from numpy.lib.histograms import histogram, histogramdd
Copy link
Member

Choose a reason for hiding this comment

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

idem., See the comment on the line above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be added to __all__ then?

Copy link
Member
@BvB93 BvB93 May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Might be worthwhile to figure out why exactly they're here in the first place.
Based on the answer deprecating (or removing) them might be desirable over adding them to __all__.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, found it: four years ago they were moved from function_base to histograms in a namespace clean up (#10186).
Honestly, this period is long enough that I'd advocate for their deprecation.

numpy/distutils/mingw32ccompiler.py Outdated
@@ -15,7 +15,6 @@
import textwrap

# Overwrite certain distutils.ccompiler functions:
import numpy.distutils.ccompiler # noqa: F401
Copy link
Member

Choose a reason for hiding this comment

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

It's possible this import is needed to do some monkey-patching. I don't know for sure.

@ajayd-san
Copy link
Contributor Author
ajayd-san commented May 25, 2021 •
edited
Loading

got it, do not remove things blindly.
I'll follow up with another pr.
Also can you tell me why are these workflow tests taking so long? I'm mean I watched a whole episode of ozark while it was still running

@eric-wieser
Copy link
Member

No need to make a new PR, you can edit this one. All of the import sys-like imports are totally safe to blindly remove.

@BvB93
Copy link
Member
BvB93 commented May 25, 2021

The changes to numpy.typing look good to me in any case.

@ajayd-san
Copy link
Contributor Author
ajayd-san commented May 25, 2021 •
edited
Loading

@eric-wieser hey, looks like travis build failed. Upon closer inspection turns out it had an error with the build script while generating it. What to do now?

@mattip
Copy link
Member
mattip commented May 25, 2021

What to do now?

Nothing, that run is flaky.

@ajayd-san
Copy link
Contributor Author

so, its going to be merged soon ?

eric-wieser
eric-wieser approved these changes May 25, 2021
View reviewed changes
Copy link
Member
@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

The non-typing bits look good to me, and it sounds like @BvB93 is happy with the typing parts.

@ajayd-san
Copy link
Contributor Author
ajayd-san commented May 25, 2021 •
edited
Loading

fine, Ill go ahead and fix unused variables.
Do i need to make a new pr or commit to this one ?

@ajayd-san
Copy link
Contributor Author

@eric-wieser shoot!!, pushed another commit which fixes unused variables on the same branch.
Is it okay ? or is it anything i can do to revert to previous commit ?
Cuz I was planning on opening another PR for unused variables as it looks a bit more organised.

@charris
Copy link
Member
charris commented May 25, 2021

anything i can do to revert to previous commit ?

git reset HEAD^  # Leaves your new work uncommitted
git stash  # Saves uncommitted work in stash
git push -f origin  # rewrites PR history

If you have the new work saved somewhere, you can just do

git reset --hard HEAD^

followed by the force push.

@charris
Copy link
Member
charris commented May 25, 2021 •
edited
Loading

You can keep pushing to the current branch as long as you want. However, at some point you may want to combine various commits using git rebase -i HEAD~<n>, where <n> is the number of commits you want to work on, followed by a force push. Google for more information about the interactive rebase (git rebase -i).

@ajayd-san
Copy link
Contributor Author

@charris Thanks for the info. I now reverted the changes to older commit. Can you confirm if its ready to merge and merge it so that i can go ahead and do another PR for the unused variables cuz the branch I created for unused variables is based on the current branch I pushed i.e default-303:LGTM_alerts not the numpy:main.

@charris
Copy link
Member
charris commented May 25, 2021

The travis error can be ignored.

@charris charris merged commit ecdba3a into numpy:main May 25, 2021
@charris
Copy link
Member
charris commented May 25, 2021

Thanks @default-303 .

@ajayd-san
Copy link
Contributor Author

welcome, thank you all for guiding me,

I've raised a new PR for unused variables, check and tell if it needs a review.

@charris charris mentioned this pull request Jun 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bashtage bashtage bashtage left review comments

@BvB93 BvB93 BvB93 left review comments

@eric-wieser eric-wieser eric-wieser approved these changes

Assignees
No one assigned
Labels
03 - Maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LGTM alerts .
6 participants
@ajayd-san @eric-wieser @BvB93 @mattip @charris @bashtage
0