typechecking poetry.core.masonry#274
Conversation
dimbleby
force-pushed
the
typechecking-masonry
branch
from
93f7187 to
955d823
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
| continue | ||
|
|
||
| if file in to_add: | ||
| # Skip duplicates |
There was a problem hiding this comment.
to_add is a set, it couldn't contain duplicates if it tried. Just need to be happy with __hash__ and __eq__ on the BuildIncludeFile.
dimbleby
force-pushed
the
typechecking-masonry
branch
from
955d823 to
d3f48dd
Compare
dimbleby
force-pushed
the
typechecking-masonry
branch
from
d3f48dd to
d57e384
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
danieleades
left a comment
danieleades
left a comment
There was a problem hiding this comment.
suggest moving the adding of mypy to a separate PR
dimbleby
force-pushed
the
typechecking-masonry
branch
4 times, most recently
from
672e316 to
a2e26b3
Compare
|
Ugh, merge conflicts. So de-motivating. I don't think this is waiting on anything, can we get it merged please? @finswimmer? (I have removed mypy from pyproject.toml in case that was controversial, though if that was a problem it's frustrating that no-one with the commit bit would say so) |
dimbleby
force-pushed
the
typechecking-masonry
branch
from
a2e26b3 to
810b42b
Compare
dimbleby
force-pushed
the
typechecking-masonry
branch
from
810b42b to
70b2a62
Compare
| target_dir: Path | None = None, | ||
| ) -> None: | ||
| super().__init__(poetry, executable=executable) | ||
| self._target_dir = target_dir |
There was a problem hiding this comment.
Not sure if this is desired. Iirc build is intended to be called multiple times with different target directories. You can default to cwd if desired though.
There was a problem hiding this comment.
The builders were inconsistent about whether they took a target directory on their constructor or on their build() method. I have made them consistently put it on the constructor.
I think the suggestion is that we should jump the other way? ie remove the parameter from the constructors, put it on the build() method?
(There's a default value either way)
There was a problem hiding this comment.
I suspect the inconsistency comes from the use of make_in in the wheel builder. Happy for it to be unified as consistency here is better, with the following funcitonality maintained.
- There is a default target directory (which as you said is present either way).
- the
build()method can accept target directory.
This can also mean that make_in / make becomes redundant. Will need to check downstream see if they are being used.
There was a problem hiding this comment.
I pushed a commit that does what I think we're agreeing - say if I've misunderstood
There was a problem hiding this comment.
make() and make_in() do indeed look redundant, happy to remove them if you want
|
@abn are we good to go? There was a question about potentially removing |
| return self.path == other.path | ||
|
|
||
| return self.path == other | ||
| def __eq__(self, other: object) -> bool: |
There was a problem hiding this comment.
Why are we disallowing comparison with Path?
There was a problem hiding this comment.
Not disallowing, but always returning False.
Because a BuildIncludeFile is not a Path!
dimbleby
force-pushed
the
typechecking-masonry
branch
from
b1acaf6 to
cb3ed03
Compare
dimbleby
force-pushed
the
typechecking-masonry
branch
from
cb3ed03 to
52597de
Compare
|
@abn are we waiting on anything to get this merged? |
dimbleby
force-pushed
the
typechecking-masonry
branch
from
52597de to
f6258d7
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
target_directory is removed from the constructor, included on `build()`
5 participants
Resolves: python-poetry#
I've added mypy to the dev-dependencies and made the poetry.core.masonry code mypy-clean.
Slightly more invasive than some of the typechecking MRs we've seen:
.build()methods have the same signature required changes (and I've removed an unused parameter too)Pathrather thanstrI've removed some odd-one-outUnion[Path, str]BuildIncludeFilecould be equal to aPathbecause it just seemed too much. I've caught one place where this trickery was being exploited, and re-implemented it in a less magical way, but it's possible that I've missed something.