Skip to content

MAINT: removed unused imports listed in LGTM#19090

Merged
charris merged 2 commits intonumpy:mainfrom
ajayd-san:LGTM_alerts
May 25, 2021
Merged

MAINT: removed unused imports listed in LGTM#19090
charris merged 2 commits intonumpy:mainfrom
ajayd-san:LGTM_alerts

Conversation

@ajayd-san
Copy link
Copy Markdown
Contributor

@ajayd-san ajayd-san commented May 25, 2021

Removed most of the unused imports mentions in LGTM.

closes #19077

eric-wieser
eric-wieser reviewed May 25, 2021
View reviewed changes
Copy link
Copy Markdown
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
import io

import abc
from abc import ABC as abc_ABC
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser May 25, 2021

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
import builtins

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

@eric-wieser eric-wieser May 25, 2021

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
Copy Markdown
Contributor

@bashtage bashtage May 25, 2021

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
Copy Markdown
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
Copy Markdown
Member

@BvB93 BvB93 May 25, 2021

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
import textwrap

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

@eric-wieser eric-wieser May 25, 2021

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
Copy Markdown
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
Copy Markdown
Member

eric-wieser commented May 25, 2021

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
Copy Markdown
Member

BvB93 commented May 25, 2021

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

@ajayd-san
Copy link
Copy Markdown
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
Copy Markdown
Member

mattip commented May 25, 2021

What to do now?

Nothing, that run is flaky.

@ajayd-san
Copy link
Copy Markdown
Contributor Author

ajayd-san commented May 25, 2021

so, its going to be merged soon ?

eric-wieser
eric-wieser approved these changes May 25, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

ajayd-san commented May 25, 2021

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

ajayd-san commented May 25, 2021

@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
Copy Markdown
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
Copy Markdown
Member

charris commented May 25, 2021

Thanks @default-303 .

@ajayd-san
Copy link
Copy Markdown
Contributor Author

ajayd-san commented May 26, 2021

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

@BvB93 BvB93 BvB93 left review comments

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

+1 more reviewer

@bashtage bashtage bashtage left review comments

Reviewers whose approvals may not affect merge requirements

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