-
-
Notifications
You must be signed in to change notification settings - Fork 32k
gh-120417: Add #noqa: F401 to tests #120627
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import_module()
can be used if ImportError is converted to SkipTest.
I have some questions about global imports of names which are seem not used in the module.
# In case _socket fails to build, make this test fail more gracefully | ||
# than an AttributeError somewhere deep in concurrent.futures, email | ||
# or unittest. | ||
import _socket | ||
import _socket # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would not using import_module()
be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent is to get an error if the module is missing, not to skip the test.
@@ -18,7 +18,7 @@ | |||
# compileall relies on ProcessPoolExecutor if ProcessPoolExecutor exists | |||
# and it can function. | |||
from multiprocessing.util import _cleanup_tests as multiprocessing_cleanup_tests | |||
from concurrent.futures import ProcessPoolExecutor | |||
from concurrent.futures import ProcessPoolExecutor # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ProcessPoolExecutor
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explained in the comment, 2 lines above. It's a check to define _have_multiprocessing
.
@@ -8,7 +8,7 @@ | |||
# of much interest anymore), and a few were fiddled to make the output | |||
# deterministic. | |||
|
|||
from test.support import sortdict | |||
from test.support import sortdict # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why sortdict
is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used in doctests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand why the import is important.
PR rebased to squash changes and fix merge conflicts. |
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand how the import is used.
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand how the import is used.
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand how the import is used.
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand how the import is used.
Ignore linter "imported but unused" warnings in tests when the linter doesn't understand why the import is important.