Skip to content

MAINT: Review F401,F841,F842 flake8 errors (unused variables and imports)#12448

Merged
charris merged 5 commits intonumpy:masterfrom
rth:dead-code
Dec 6, 2018
Merged

MAINT: Review F401,F841,F842 flake8 errors (unused variables and imports)#12448
charris merged 5 commits intonumpy:masterfrom
rth:dead-code

Conversation

@rth
Copy link
Contributor

@rth rth commented Nov 25, 2018

In this PR I run the flake8 on checks corresponding to unused imports and variables,

flake8 --select=F401,F841,F842 numpy

and selectively removed those I thought should be harmless. For unused variables that looked suspicious, a FIXME comment was added.

Unused imports is an issue beyond code style as it makes dependencies between modules harder to understand (not that there much issues with that in numpy -- this PR mostly changes test files).

This should also help with LGTM.com warnings.

@rth rth changed the title Review F401,F841,F842 flake8 errors (unused variables and imports) MAINT: Review F401,F841,F842 flake8 errors (unused variables and imports) Nov 25, 2018
# Regression test for gh-7707. If npymath.ini wasn't installed, then this
# will give an error.
info = get_info('npymath')
get_info('npymath')
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to add some basic test of info here, rather than just ignoring the return value

@eric-wieser
Copy link
Member

Mostly looks good. Thanks for only touching test and private files - note that in many cases we're stuck with unused imports in our public files because those import often accidentally form part of our public API.

@rth
Copy link
Contributor Author

rth commented Nov 25, 2018

Thanks for the review! I addressed your comments, added a few FIXME tags for things that you marked as potentially suspicious and that I didn't want to investigate in this PR.

note that in many cases we're stuck with unused imports in our public files because those import often accidentally form part of our public API.

Yes, it's not an easy problem...

except AttributeError:
return False
# FIXME: below code is never reached
__NUMPY_SETUP__ = False
Copy link
Member

Choose a reason for hiding this comment

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

Ok, I get this now - I think this is supposed to set builtins.__NUMPY_SETUP__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but e.g. numpy/__init__.py handles the existence of builtins.__NUMPY_SETUP__ as true, and the absence of it as false. I removed this, as this line is never reached and I think it's more confusing than it helps.

Also, unless I missed something, this util function doesn't seem to be called in the numpy code in general, and it doesn't really make sense to call it for other packages I think?

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it out, it hasn't been missed since 2009.

@charris
Copy link
Member

charris commented Dec 4, 2018

@rth Ping.

@rth
Copy link
Contributor Author

rth commented Dec 4, 2018

Thanks for the reminder @charris . Addressed @eric-wieser 's comments.

@charris charris added this to the 1.16.0 release milestone Dec 5, 2018
@charris charris merged commit 45cef38 into numpy:master Dec 6, 2018
@charris
Copy link
Member

charris commented Dec 6, 2018

Thanks @rth .

@rth
Copy link
Contributor Author

rth commented Dec 6, 2018

Thanks for the review @eric-wieser and @charris !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants