-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: qualifiers type annotation #172
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jonathan Howard <[email protected]>
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 good and locally it passes mypy
😄
@jhoward-lm I see there are still |
@tdruez I don't believe so. Those are generalized argument/parameter types, but the type gets narrowed before returning. You did however draw my attention to several other typing issues; I'll push a commit shortly for review |
Signed-off-by: Jonathan Howard <[email protected]>
Signed-off-by: Jonathan Howard <[email protected]>
src/packageurl/__init__.py
Outdated
@@ -546,4 +541,4 @@ def from_string(cls, purl: str) -> "PackageURL": | |||
encode=False, | |||
) | |||
|
|||
return PackageURL(type, namespace, name, version, qualifiers, subpath) | |||
return cls(type, namespace, name, version, qualifiers, subpath) # 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.
you can easily fix this, then I don't need to create a separate PR 😅
Just create your own AnyStr
type alias and remove the one coming from the typing
module.
Add
AnyStr = Union[str, bytes]
directly under basestring
at the top and remove the from typing import AnyStr
. AnyStr
was deprecated in Python 3.13 anyway.
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.
Good call! One small alteration: since it's only used for type hints, I put it into the TYPE_CHECKING
block. Tests still pass locally. Are you good with 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.
sure, also sounds good
Signed-off-by: Jonathan Howard <[email protected]>
@gruebel @tdruez So things like this could be updated across the board: # before
Union[str, Dict[str, str], None]
# after
str | dict[str, str] | None Let me know if you want me to go ahead with it, or I can hold off. |
I actually like the newer syntax style more than the old one, but I would recommend to do it in a follow-up PR, because it is kind of like formatting and I don't like to mix actual changes with formatting. |
This PR modifies the type annotation of the
qualifiers
attribute declaration from:to:
in order to reflect its true runtime type.
Closes #169.
Miscellaneous
encode: "Optional[Literal[False]]"
overload ofnormalize_qualifiers
toDict[str, str]
encode: "Optional[Literal[False]]"
overload ofnormalize
toDict[str, str]
(the other tuple members in the return type unchanged)Union[AnyStr, Dict[str, str], None]
toOptional[Union[AnyStr, Dict[str, str]]]
Union[str, Dict[str, str], None]
toOptional[Union[str, Dict[str, str]]]
# NOQA
directivesPackageURL.__new__
andPackageURL.from_string
totyping_extensions.Self
PackageURL.__new__
fromself
tocls
since it is aclassmethod
from __future__ import annotations