-
Notifications
You must be signed in to change notification settings - Fork 56
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
Avoid Mypy error when there's an __init__.py
in the repo root
#499
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.
Looks great! Thanks for picking this up so quickly. And sorry for the lag - #timezones, plus I had to learn how to use github codereview haha.
src/darker/tests/test_linting.py
Outdated
def __init__( | ||
self, | ||
args: List[str], | ||
stdout: int, | ||
encoding: str, | ||
cwd: Path, | ||
env: Dict[str, str], | ||
): | ||
super().__init__( | ||
args, stdout=stdout, stderr=PIPE, encoding=encoding, cwd=cwd, env=env | ||
) |
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.
Not sure if it fits well with your prevailing style (or typechecking - I'm still learning how to bend mypy to my will :p), but I think you could do
def __init__( | |
self, | |
args: List[str], | |
stdout: int, | |
encoding: str, | |
cwd: Path, | |
env: Dict[str, str], | |
): | |
super().__init__( | |
args, stdout=stdout, stderr=PIPE, encoding=encoding, cwd=cwd, env=env | |
) | |
def __init__(self, args: List[str], **kwargs): | |
kwargs['stderr'] = PIPE | |
super().__init__(args, **kwargs) |
(this works for Popen because it explicitly declares its trailing parameters to be keyword-only)
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.
I originally had *args, **kwargs
but Mypy wasn't happy for it. The options were to omit types and do a #type: ignore
, or to separate and annotate the keyword arguments.
I guess for unit tests and such a minimal helper skipping type checking would be just fine. Would you prefer that?
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.
Actually this was trivial to do. The class is now much more concise.
src/darker/tests/test_linting.py
Outdated
|
||
|
||
def test_get_messages_from_linters_for_baseline_no_mypy_errors(git_repo, capsys): | ||
"""Test for `linting._get_messages_from_linters_for_baseline`""" |
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.
nit: Should it be something like
"""Test for `linting._get_messages_from_linters_for_baseline`""" | |
"""Ensure mypy does not fail early when __init__.py is at the repo root""" |
in our codebase we sometimes also mention the bug ID (#498) in a comment somewhere. E.g.,
# Regression test for #498.
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.
I admit my unit test docstring convention isn't always too useful. I fixed that according to your suggestion.
flakes/py312/flake.nix
Outdated
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.
not sure I understand what this file is for (could it be a stray untracked file that got added?)
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.
Removed, accidental. Was related to getting Python 3.12.dev running on my NixOS laptop.
5a2d820
to
eb41d5c
Compare
eb41d5c
to
abc9515
Compare
Asserts that Mypy doesn't output anything on stderr. This currently fails when running Mypy for the baseline, because the temporary directory name contains a hyphen which makes it an invalid Python package name.
Use the original repository root directory name. This way Mypy behaves similarly to the original working directory and doesn't fail when there's an `__init__.py` in the root.
abc9515
to
056ff47
Compare
Phew, I think I've now dealt with Python<3.9, Mypy, Pylint and Bandit and this can be merged. I rebased the modifications into the original commits. Gotta say, despite the trouble, I still do value highly linters, good test coverage and especially code reviews. Thanks @tapted! |
Fixes #498.