Fix and enable flake8 error "imported but unused" #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
timokau
commented
Mar 30, 2020
csrank/__init__.py
Outdated
| from .tunable import Tunable | ||
| from .tuning import ParameterOptimizer | ||
|
|
||
| __all__ = ["Tunable", "ParameterOptimizer"] | ||
| __all__.extend(choicefunction) |
There was a problem hiding this comment.
I'm unsure about this. We could also use # noqa to just skip the check here. Maybe we should reconsider whether re-exporting everything at the top level is even desirable.
There was a problem hiding this comment.
Also looks like the current solution is not working as expected:
ImportError while importing test module '/home/travis/build/kiudee/cs-ranking/csrank/tests/test_choice_functions.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
csrank/tests/test_choice_functions.py:27: in <module>
from csrank.tests.test_ranking import check_params_tunable
csrank/tests/test_ranking.py:8: in <module>
from csrank import PairedCombinatorialLogit
E ImportError: cannot import name 'PairedCombinatorialLogit' from 'csrank' (/home/travis/build/kiudee/cs-ranking/csrank/__init__.py)
There was a problem hiding this comment.
Regarding the second comment: I think in tests it is better to import using the full path anyway.
There was a problem hiding this comment.
As discussed on slack, we might want to reduce the re-exports in the future but this is not the place to do it. So I'm going with # noqa comments for now.
timokau
force-pushed
the
flake8-f401
branch
3 times, most recently
from
c396ba3 to
b674fb1
Compare
|
Weird, I'm pretty sure I ran the The travis issue should be fixed now. Ready for review. |
As suggested by pep8: https://www.python.org/dev/peps/pep-0008/#public-and-internal-interfaces Not doing so trips up flake8's F401, since the imports are not used. Imports are an implementation detail and should not directly be relied upon to declare the public API. Two exceptions to this are csrank/__init__.py and csrank/dataset_reader/__init__.py, where the API is too big. This will likely change in the future. See kiudee#105.
We have now fixed all occurrences of this issue.
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
==========================================
+ Coverage 59.15% 59.22% +0.06%
==========================================
Files 116 116
Lines 7475 7472 -3
==========================================
+ Hits 4422 4425 +3
+ Misses 3053 3047 -6
|
kiudee
approved these changes
Apr 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Continuation of #104.
Motivation and Context
How Has This Been Tested?
Does this close/impact existing issues?
Types of changes
Checklist: