style: pylint #3720
style: pylint #3720
Conversation
setup.py
Outdated
| @@ -41,12 +41,10 @@ def build_expected_version_hex(matches: Dict[str, str]) -> str: | |||
| serial = int(level_serial[len(level) :]) | |||
| break | |||
| if serial is None: | |||
| msg = 'Invalid PYBIND11_VERSION_PATCH: "{}"'.format(patch_level_serial) | |||
| msg = f'Invalid PYBIND11_VERSION_PATCH: "{patch_level_serial}"' | |||
There was a problem hiding this comment.
I would drop fstrings from here, they will just cause syntax errors if someone tries to pip install from an old setup.py before they hit the version checks, right?
There was a problem hiding this comment.
(mentioned elsewhere, but repeated more or less here) There are no version checks in setup.py. The Requires-Python setting (in setup.cfg) should keep this from running on an older version. And an f-string failure is a pretty good way to fail - most people know f-strings mean Python 3.6 (it's often the first failure).
.pre-commit-config.yaml
Outdated
| @@ -76,8 +82,7 @@ repos: | |||
| - id: python-check-blanket-noqa | |||
| - id: python-check-blanket-type-ignore | |||
| - id: python-no-log-warn | |||
| # Python 3.6 | |||
| # - id: python-use-type-annotations | |||
| - id: python-use-type-annotations | |||
There was a problem hiding this comment.
You can change a mypy setting to have it complain about these too.
henryiii
force-pushed
the
henryiii/style/pylint
branch
from
c10c00a
to
be9a98a
Compare
|
Ready whenever any/every one else is. Does include a fix for a recent regression in intree helpers. |
| @@ -1,3 +1,5 @@ | |||
| # pylint: disable=missing-function-docstring | |||
There was a problem hiding this comment.
I think it would be better to move this to the messages_control.disable list in pyproject.toml.
I often see people writing completely redundant (useless) docstrings just to appease the linter. In my experience it is better left for the review stage, requesting docstrings as needed, if something isn't clear. (Even then, I usually encourage clearer variable and function naming over adding docstrings.)
There was a problem hiding this comment.
We already have docstrings on everything except __main__ (where they are very much not needed), and I don't expect this to change much. I did disable the module docstring one, as that would be overkill. How about we only disable it (further) if it gets in the way?
There was a problem hiding this comment.
It's fine.
Internally we have our linter configured to warn about function docstrings only if the function is longer than N lines (10 maybe, not sure). I find that useful.
But we can revisit as needed.
Description
Builds on #3719, adds pylint, which discovered a mistake in the recent reworking of intree helpers.
Suggested changelog entry:
None needed.