-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
CI: Pylint 3.x #3110
base: master
Are you sure you want to change the base?
CI: Pylint 3.x #3110
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.
Would it be possible to just extract the changes to the Python version support and skip the rest, please? I'm not seeing the(pylint comments, inotify import changes, or signal import renaming as relevant to the PR description.
|
@pajod I'm happy to review the linting-related changes in this PR or separate, your choice. Let's just get the pypy version fixed up and I think we're good to go. |
Is there a timeline when this will be merged? |
3b665a4
to
f950046
Compare
4f21dea
to
c613240
Compare
@pajod why a change to the reloadedr is part of this change? |
Pylint 3.0 is the first version to officially support Python 12
Debian buster EoL since 2022-09-10
@benoitc 2nd attempt for comment marked "outdated" above. I want newer pylint versions to stop suggesting that inotify might be undefined in the separate conditional, without suppressing that (generally useful!) detection too much:
The previous version you commented on moved the alternate reloader implementation to an extra file, so the imports are seen as unconditional (within that file) to static analysis. Most recent rebased version is the simple solution without extra file again by duplicating the import: much smaller diff for typically similar cost. |
To fix those two pylint errors, you can change Line 63 in 7268a61
to read inotify = None
|
if sys.platform.startswith('linux'): | ||
try: | ||
import inotify.constants as _ | ||
from inotify.adapters import Inotify | ||
import inotify.constants | ||
has_inotify = True | ||
except ImportError: | ||
pass |
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 other changes are not needed to placate pylint.
pass | |
inotify = None |
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.
Almost, because we have two conditionals (the remainder of the sys.platform branch must be covered aswell) and import two things. But while mypy hates this, pylint seems OK with being tricked by the two-line version, see https://github.com/benoitc/gunicorn/pull/3110/files#diff-f807d81a0dfb3d826a7b566e4902c676f12447b6d5c442d4c291edca727b6cb5
Included:
Removed:
Not included:
Suggested order: Merge #3148 or equivalent first, possibly eradicating the identifier clash in a simpler manner.