Add type validation #294
Add type validation #294
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: francisbergin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ghost
mentioned this pull request
jwhonce
left a comment
jwhonce
left a comment
There was a problem hiding this comment.
@francisbergin I like what I see on the whole. I am interested in what you find concerning the convenience methods. Changing delete() seems like an error in the type checker.
| self, | ||
| path: Union[str, bytes], | ||
| params: Union[None, bytes, Mapping[str, str]] = None, | ||
| params: Union[None, bytes, Mapping[str, Any]] = None, |
There was a problem hiding this comment.
This change seems spurious as none of the other like methods were flagged, ie get/put/head/... These methods all call _request() which has params: Union[None, bytes, Mapping[str, str]] = None,
There was a problem hiding this comment.
This change was to fix this error:
podman/domain/networks_manager.py:202: error: Dict entry 0 has incompatible type "str": "bool | None"; expected "str": "str" [dict-item]
This is the call to delete in that file:
response = self.client.delete(f"/networks/{name}", params={"force": force})
Where force is typed Optional[bool].
There was a problem hiding this comment.
So the type checker didn't flag the delete method directly; it flagged the call to it which did not follow the type hints.
There was a problem hiding this comment.
Ah, should the caller be fixed instead?
There was a problem hiding this comment.
I think it is reasonable to call client.delete("...", params={"force": True} so supporting a bool value should be good.
However, I'm not sure the use of Mapping[str, Any] is the best approach. Since the value for params is sent directly to requests.Session.request, the most appropriate supported type would be that one. Typeshed defines it as:
_ParamsMappingKeyType: TypeAlias = str | bytes | int | float
_ParamsMappingValueType: TypeAlias = str | bytes | int | float | Iterable[str | bytes | int | float] | None
_Params: TypeAlias = (
SupportsItems[_ParamsMappingKeyType, _ParamsMappingValueType]
| tuple[_ParamsMappingKeyType, _ParamsMappingValueType]
| Iterable[tuple[_ParamsMappingKeyType, _ParamsMappingValueType]]
| str
| bytes
)But this seems like a lot. Also, if I understand correctly, using Any disables type verification for the specific variable.
Maybe we can just add support for bool to allow for the specific usage and adapt later as required?
params: Union[None, bytes, Mapping[str, Union[str, bool]]] = None,There was a problem hiding this comment.
I am comfortable with that change, as long as it is propagated to all the http verb helper functions, ie get/post/etc
Signed-off-by: Francis Bergin <francisbergin@hotmail.com>
|
@jwhonce The gating tests are now passing with type hint validation! However, I think this PR still needs a bit of work:
There are a lot of changes required to get mypy to pass but I believe it brings a lot of value to the project. |
| if isinstance(path, bytes): | ||
| path = path.decode() |
There was a problem hiding this comment.
@jwhonce Since path can be of type bytes, converting it to a string is required to get .lstrip(str) to be valid. Without conversion, a TypeError would be thrown on a bytes path. It seems all calls to APIClient are done within the package and all use strings for the path parameter. Is so, specifying path as str only would remove the need for a conversion here.
| return self.name | ||
|
|
||
| @cached_property | ||
| @cached_property # type: ignore[operator] |
There was a problem hiding this comment.
For some reason, mypy thinks cached_property is a module and fails with a "module not callable" error.
| if TYPE_CHECKING: | ||
| from podman.domain.images_manager import ImagesManager |
| PodmanResourceType: TypeVar = TypeVar("PodmanResourceType", bound="PodmanResource") | ||
| # pylint: disable=invalid-name | ||
| PodmanResourceType = TypeVar("PodmanResourceType", bound="PodmanResource") |
There was a problem hiding this comment.
I'm not 100% confident in the PodmanResource/PodmanResourceType changes in this module.
| return self.manager.pull(repository, tag=self.id, platform=platform) | ||
| return self.manager.pull(repository[0], tag=self.id, platform=platform) |
There was a problem hiding this comment.
api.parse_repository returns a tuple but ImagesManager.pull takes a string as first argument.
| version = self.client.version() | ||
| version = self.client.version() # type: ignore[operator] |
There was a problem hiding this comment.
Here self.client is a APIClient where .version is a string. It would be in PodmanClient that .version would be callable. This specific line should throw an error as it is.
|
@francisbergin any updates here? |
|
Hi! No, I didn't have time to continue working on this. I still think it would be good to have but the PR still needs some work and I don't have much time now. Maybe another time! |
3 participants
No description provided.