Closed chludwig-haufe closed 11 months ago
I found that piptools.utils
already contained a function strip_extras()
(line 478). Therefore, I switched to using the existing function. I don't see any reason to duplicate this functionality.
I wasn't aware of that function. Say, also since it's already used in resolver.py, wouldn't it be easier to just use it on the output of key_from_req
? What do you think?
Sorry for pushing you around.
No problem, sometimes implementations need multiple iterations before the best approach becomes apparent.
I will take a look in the evening how to simplify the PR this way.
@atugushev: I replaced the unit test with an integration test as you proposed.
I gather from pyproject.toml
(line 95ff) that your intention is to ignore untyped definitions in the tests. For some reason, mypy called from the pre-commit hook insists on typed definitions on all functions I touched in the tests, including the functions passed in as fixtures.
These functions have "complex" signatures (too complex for Callable
, at least), whence I had to define protocol classes for them. Furthermore, I cannot reference these protocols if I simply define them next to the fixtures in conftest.py
. Therefore, I placed them into tests/utils.py
.
I gather from
pyproject.toml
(line 95ff) that your intention is to ignore untyped definitions in the tests. For some reason, mypy called from the pre-commit hook insists on typed definitions on all functions I touched in the tests, including the functions passed in as fixtures.
This however sounds like an issue that should be investigated. Haven't found something loadable yet, though.
@chrysle: For demonstration's sake, I added --verbose
to the mypy arguments in .pre-commit-config.yaml and then removed the type hint for the parameter make_package
of test_resolver_drops_existing_conflicting_constraint()
in test_cli_compile.py
.
pre-commit rejected this commit, here's the relevant output:
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1
LOG: Mypy Version: 1.5.1
LOG: Config File: pyproject.toml
LOG: Configured Executable: /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/bin/python
LOG: Current Executable: /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/bin/python
LOG: Cache Dir: .mypy_cache
LOG: Compiled: True
LOG: Exclude: ['^tests/test_data/']
LOG: Found source: BuildSource(path='tests/test_cli_compile.py', module='tests.test_cli_compile', has_text=False, base_dir='/Users/christoph.ludwig/Python/pip-tools', followed=False)
LOG: Could not load cache for tests.test_cli_compile: tests/test_cli_compile.meta.json
LOG: Metadata not found for tests.test_cli_compile
LOG: Parsing tests/test_cli_compile.py (tests.test_cli_compile)
LOG: Metadata fresh for tests: file /Users/christoph.ludwig/Python/pip-tools/tests/__init__.py
LOG: Metadata fresh for piptools.scripts.compile: file /Users/christoph.ludwig/Python/pip-tools/piptools/scripts/compile.py
LOG: Metadata fresh for unittest.mock: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/mypy/typeshed/stdlib/unittest/mock.pyi
LOG: Metadata fresh for click.testing: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/click/testing.py
LOG: Metadata fresh for piptools.utils: file /Users/christoph.ludwig/Python/pip-tools/piptools/utils.py
LOG: Metadata fresh for tests.constants: file /Users/christoph.ludwig/Python/pip-tools/tests/constants.py
LOG: Metadata fresh for tests.utils: file /Users/christoph.ludwig/Python/pip-tools/tests/utils.py
[ ... many more metadata refresh messages ... ]
LOG: Metadata fresh for packaging._elffile: file /Users/christoph.ludwig/.cache/pre-commit/repo3zt33lob/py_env-python3.11/lib/python3.11/site-packages/packaging/_elffile.py
LOG: Loaded graph with 249 nodes (0.058 sec)
LOG: Found 149 SCCs; largest has 39 nodes
LOG: Processing 148 queued fresh SCCs
LOG: Processing SCC singleton (tests.test_cli_compile) as inherently stale
tests/test_cli_compile.py:2874: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
LOG: Deleting tests.test_cli_compile tests/test_cli_compile.py tests/test_cli_compile.meta.json tests/test_cli_compile.data.json
LOG: No fresh SCCs left in queue
LOG: Build finished in 0.253 seconds with 249 modules, and 1 errors
Found 1 error in 1 file (checked 1 source file)
pre-commit rejected this commit, here's the relevant output:
I guess that's unrelated; it's just rebuilding its cache.
Could you try if setting disallow_incomplete_defs
to false in the mypy
override works for you? I have the strong feeling that could be the culprit.
Could you try if setting
disallow_incomplete_defs
to false in themypy
override works for you? I have the strong feeling that could be the culprit.
Right, with disallow_incomplete_defs = false
in the test override, mypy / pre-commit let the commit pass.
What do you want me to do about the annotations (and, in particular, the protocol classes) I added to the tests? Should I take them out again (or, maybe, move the protocol classes into a dedicated module, because they don't really fit into utils
)?
What do you want me to do about the annotations (and, in particular, the protocol classes) I added to the tests? Should I take them out again
I think that would be the better solution, since it's otherwise unnecessary complex.
I think that would be the better solution, since it's otherwise unnecessary complex.
Done.
Thank you!
This PR resolves #1977.
When the backtracking resolver collected the names of incompatible install requirements, it used to apply
utils.key_from_req
to the requirements listed in the exception causes. According to the type hints,utils.key_from_req
expects either apip._internal.req.req_install.InstallRequirement
or apip._vendor.packaging.requirements.Requirement
. In some cases, though, the object passed is apip._internal.resolution.resolvelib.requirements.SpecifierRequirement
(i.e., a specialization ofpip._internal.resolution.resolvelib.base.Requirement
).All mentioned classes define an attribute
name
. In case ofInstallRequirement
and theRequirement
from the vendor package, the name holds the bare package name. In case of theRequirement
from the resolvelib package, the name includes the extras (if any).key_from_req
is used in other places as well, I don't know whether they rely on the existing behavior. Therefore, I added a new functionutils.key_no_extra_from_req
that also accepts instances ofpip._internal.resolution.resolvelib.base.Requirement
and keeps only the part of the name up to the first opening square bracket (if any).I was surprised I did not find more tests of the backtracking resolver. In the end, I followed the example of
test_catch_distribution_not_found_error
and directly called the backtracking resolver's method_do_resolve
with a mocked pip resolver object.Furthermore, I had to add pytest to the config of the mypy-mirror pre-commit hook.
I still got "untyped def" errors from
test_resolver.py
, even though there's an override for test modules in pyproject.toml that is supposed to disable this check. I therefore had the impression that mypy-mirror ignores the mypy config in pyproject.toml. I didn't investigate this any further, though, and simply annotated the all parameters of my test.Contributor checklist
Maintainer checklist
backwards incompatible
,feature
,enhancement
,deprecation
,bug
,dependency
,docs
orskip-changelog
as they determine changelog listing.