Cleanup the python module #8992
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8992 +/- ##
==========================================
- Coverage 66.42% 63.29% -3.14%
==========================================
Files 374 187 -187
Lines 84329 41945 -42384
Branches 17557 8737 -8820
==========================================
- Hits 56018 26548 -29470
+ Misses 23387 12990 -10397
+ Partials 4924 2407 -2517
Continue to review full report at Codecov.
|
dcbaker
force-pushed
the
submit/modernize-python-module-dependency
branch
from
c416642
to
74ad9b6
Compare
There's still a number of things that don't properly type check, that's expected though as the input is often unvalidated and assumed good.
And note that the way that find_installation works is completely broken in regards to machine files
Including not calling back into `Interpreter.func_*`, which is not a good idea both from a type saftey and perforamance point of view. Instead there's now a shared _impl method
This removes the odd 'pkgdep' attribute thing, and makes it behave more like a proper dependency
Nothing uses this anymore, so don't check for it.
Both of these are artifacts of the time before Dependency Factories, when a dependency that could be discovered multiple ways did ugly stuff like finding a specific dependency, then replacing it's own attributes with that dependency's attributes. We don't have cases of that left in the tree, so let's get rid of this code too
These were spotted by mypy and pyright. One is a string where a Path is expected, another other is a possibly unbound variable, and the third is bound but unused variables.
dcbaker
force-pushed
the
submit/modernize-python-module-dependency
branch
from
74ad9b6
to
b4e826b
Compare
|
Anyone object to merging this? I have a series to use type checking in the pkg-config module that depends on this (and getting rid of the .pc_dep stuff) |
| if T.TYPE_CHECKING: | ||
| _Base = ExternalDependency | ||
| else: | ||
| _Base = object | ||
|
|
||
| class _PythonDependencyBase(_Base): |
There was a problem hiding this comment.
Is there a specific reason for doing this (cyclic imports?) or is this just for improving imports?
There was a problem hiding this comment.
It helps mypy, which doesn't understand these kind of mixing classes at all. So we get two choices, forward declare a bunch of methods and attributes in the mixing class and try to keep them in sync, or this, which has mypy understand this as an ExternalProgram, but at runtime avoids the two bases problem.
There was a problem hiding this comment.
I use the same trick in the compilers class.
I wanted to get us down to one set of python dependency classes, but that's still not achieved here. What is achieved is a complete set of type annotations for the python module (there's still some places where things fail because we need more typing in the interpreter), the use of the
typed_*decorators where applicable, and the conversion of the megaPythonDependencyto a dependency factory. The use of a factory allows us to drop a bunch of legacy cruft that had been mostly purged, but was sitll around because of this one dependency.@xclaesse I dropped the test that checked the pkg_config.pcdep test, but I'm not sure if you were specifically trying to test
.pcdepor if that was just the method you used to test the module itself. If the latter I can rewrite the test instead.