-
Notifications
You must be signed in to change notification settings - Fork 251
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
chore(packages): remove type errors #339
Conversation
new._constraint = constraint | ||
new._pretty_constraint = str(constraint) | ||
|
||
new.set_constraint(constraint) |
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.
before, this allowed constraint
to be a str
, although it seems the intention was for set_constraint
to handle normalizing to VersionConstraint
(as well as setting _pretty_constraint
)
def set_constraint(self, constraint: str | VersionConstraint) -> None: |
e550f4e
to
f61fbe2
Compare
f61fbe2
to
7b57b91
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
|
||
for marker in markers: | ||
if or_: | ||
groups.append([]) | ||
|
||
if isinstance(marker, (MultiMarker, MarkerUnion)): | ||
groups[-1].append( | ||
group_markers(marker.markers, isinstance(marker, MarkerUnion)) | ||
group_markers(marker.markers, isinstance(marker, MarkerUnion)) # type: ignore[arg-type] |
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 got stuck here (in combination with the changed return type), but maybe not worth thinking about it since group_markers
is not required anymore with #347. (The output of it seems to be in a weird form for representing or
anyway.)
@branchvincent Can you rebase, please? (Sorry for the merge conflict.) 🙏 |
@@ -538,6 +533,7 @@ def create_from_pep_508( | |||
link = Link(path_to_url(p)) | |||
|
|||
# it's a local file, dir, or url | |||
dep: Dependency | None |
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.
suggestion: initialize the value to None
here, rather than later.
@@ -558,7 +554,7 @@ def create_from_pep_508( | |||
name = m.group("name") | |||
version = m.group("ver") | |||
|
|||
name = req.name or link.egg_fragment | |||
name = cast(str, req.name or link.egg_fragment) |
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.
it's not clear what the intention of the (original) code is here: the name extracted only three lines previously is discarded here. So something has gone wrong and it would be good to take the opportunity to fix it.
My guess is that this whole fragment can be discarded (anyway that doesn't break any tests).
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.
(and if that's right then there are no users of egg_fragment
left so maybe that can be discarded too)
self.readmes: tuple[Path, ...] = () | ||
|
||
self.extras = {} | ||
self.requires_extras = [] | ||
self.extras: dict[str, list[str | Dependency]] = {} |
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.
pretty sure this should be dict[str, list[Dependency]]
@@ -164,10 +168,10 @@ def group_markers( | |||
def convert_markers(marker: BaseMarker) -> dict[str, list[list[tuple[str, str]]]]: | |||
groups = group_markers([marker]) | |||
|
|||
requirements = {} | |||
requirements: dict[str, list[list[tuple[str, str]]]] = {} |
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.
this is ConvertedMarkers
isn't it?
@@ -412,7 +407,7 @@ def _create_nested_marker( | |||
if not constraint.include_min: | |||
op = ">" | |||
|
|||
version = constraint.min.text | |||
version: Version | str = constraint.min.text |
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.
or I think this can always be a string (without needing annotation) if you set version = constraint.max.text
a few lines later
Closing this in favor of #354. |
Removes
poetry.core.packages.*
from the ignored listRelates-to: python-poetry/poetry#5017