-
Notifications
You must be signed in to change notification settings - Fork 24
Add type information to pushsource #207
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
base: master
Are you sure you want to change the base?
Changes from all commits
7e0a42c
e57b42c
1a85b56
48b25a1
9636c86
66a4172
b07c9db
e920670
f80fc7d
a995b4a
67585ba
371485e
d9dc2f2
4fcd047
d791837
4c9df5d
9096672
0cae6ff
ecf2852
02e6101
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import xmlrpc.client | ||
| from collections import Sequence | ||
| from typing import Any, TypeVar | ||
|
|
||
| from pushsource._impl.type_aliases import JsonObject | ||
|
|
||
| ErrataRaw_co = TypeVar("ErrataRaw_co", bound="ErrataRaw", covariant=True) | ||
|
|
||
| class ErrataRaw(object): | ||
| advisory_cdn_metadata: JsonObject | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming here with JsonObject doesn't quite seem right since the API being used here is not JSON-based. It's XML-RPC. I don't know if there's even a proper standard, but in the python world at least the marshalling behavior is documented at https://docs.python.org/3/library/xmlrpc.client.html and it doesn't precisely match JSON I think, though it's probably close. Would it make more sense to call this something like ApiObject or ErratumApiObject? |
||
| advisory_cdn_file_list: JsonObject | ||
| advisory_cdn_docker_file_list: JsonObject | ||
| ftp_paths: JsonObject | ||
| def __init__( | ||
| self, | ||
| advisory_cdn_metadata: JsonObject, | ||
| advisory_cdn_file_list: JsonObject, | ||
| advisory_cdn_docker_file_list: JsonObject, | ||
| ftp_paths: JsonObject, | ||
| ) -> None: ... | ||
|
|
||
| class ErrataClient(object): | ||
| _errata_service: xmlrpc.client.ServerProxy | ||
| def __init__(self, threads: int, url: str, **retry_args: Any) -> None: ... | ||
| def shutdown(self) -> None: ... | ||
| def _log_queried_et(self, response: JsonObject, advisory_id: str) -> JsonObject: ... | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious does it cause problems if some of these are left out? I don't object to people adding type hints for whatever they want, but I don't want to require that type hints must be added even for private methods on private classes. So hopefully if, for example, a developer were to add new private methods on ErrataClient and not add them to the type hints, it wouldn't break anything.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It doesn't cause problems to leave things out from the stub files in the sense that there's no issue at runtime. However, it is worth noting that IDEs will likely handle this in different ways. Using PyCharm as an example, it emits an unresolved reference Warning for In the first case the warning is easily remedied by leaving the function signature in the stub file, but removing the type hints from it, which could be a happy middle ground. This would look like: def _log_queried_et(self, response, advisory_id): ...This approach would attempt to keep the type information private to anyone who reads the stub file, but also prevents the unresolved reference warning.
I agree with this sentiment, and it would be worth documenting that type hints are not required in the README or published library docs for contributing to the library that this is the case. |
||
| def get_raw_f(self, advisory_id: str) -> Sequence[ErrataRaw_co]: ... | ||
| # TODO: narrow return type if possible: JsonObject maybe? | ||
| def _call_et(self, method_name: str, advisory_id: str) -> Any: ... | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| from collections import Mapping, Iterator | ||
| from collections.abc import Sequence | ||
| from types import TracebackType | ||
| from typing import Optional, Type, List, Any | ||
|
|
||
| from pushsource import ( | ||
| Source, | ||
| ErratumPushItem, | ||
| ContainerImagePushItem, | ||
| ) | ||
| from pushsource._impl.backend.errata_source.errata_client import ErrataRaw | ||
| from pushsource._impl.model.base import PushItem_co, PushItem_contra | ||
| from pushsource._impl.type_aliases import MaybeString | ||
|
|
||
| # TODO: is the value type a model type or just a Mapping? | ||
| DockerFileList = Mapping[str, Any] | ||
|
|
||
| # TODO: is the value type a model type or just a Mapping? | ||
| RpmList = Mapping[str, Mapping[Any, ...]] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel it is not right to try to model these accurately because the data at this point is still untrusted, I believe. If I understand right, these types are being used on the raw data within ET responses and we may expect them to be of a certain type, but we don't actually know that they are. To me then it implies these should stay as the generic types like JsonObject (or ApiObject or whatever if it's renamed). With RpmList for instance, if a function receives an RpmList which is a |
||
|
|
||
| class ErrataSource(Source): | ||
| _errata_service_url: str | ||
| _advisory_ids: List[str] | ||
| def __init__( | ||
| self, | ||
| url: str, | ||
| errata: MaybeString, | ||
| koji_source: Optional[str] = ..., | ||
| rpm_filter_arch: Optional[MaybeString] = ..., | ||
| legacy_container_repos: bool = ..., | ||
| threads: int = ..., | ||
| timeout: int = ..., | ||
| ) -> None: ... | ||
| def __enter__(self) -> "ErrataSource": ... | ||
| def __exit__( | ||
| self, | ||
| exc_type: Type[BaseException], | ||
| exc_val: BaseException, | ||
| exc_tb: TracebackType, | ||
| ) -> None: ... | ||
| def __iter__(self) -> Iterator[PushItem_co]: ... | ||
| def _koji_source(**kwargs: Any) -> Source: ... | ||
| def _push_items_from_raw(self, raw: ErrataRaw) -> Sequence[PushItem_co]: ... | ||
| def _push_items_from_container_manifests( | ||
| self, erratum: ErratumPushItem, docker_file_list: DockerFileList | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _enrich_container_push_item( | ||
| self, | ||
| erratum: ErratumPushItem, | ||
| docker_file_list: DockerFileList, | ||
| item: ContainerImagePushItem, | ||
| ) -> ContainerImagePushItem: ... | ||
| def _push_items_from_rpms( | ||
| self, erratum: ErratumPushItem, rpm_list: RpmList | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _module_push_items_from_build( | ||
| self, erratum: ErratumPushItem, build_nvr: str, build_info: Mapping[Any, ...] | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _filter_rpms_by_arch( | ||
| self, erratum: ErratumPushItem, rpm_filenames: Sequence[str] | ||
| ) -> Sequence[str]: ... | ||
| def _rpm_push_items_from_build( | ||
| self, erratum: ErratumPushItem, build_nvr: str, build_info: Mapping[Any, ...] | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _add_ftp_paths( | ||
| self, items: Sequence[PushItem_contra], erratum: ErratumPushItem, raw: ErrataRaw | ||
| ) -> Sequence[PushItem_co]: ... | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| from collections import Mapping, Sequence | ||
| from typing import Final, Any | ||
|
|
||
| from pushsource import ContainerImagePullInfo | ||
| from pushsource._impl.model.container import ContainerImagePullSpec_co | ||
|
|
||
| MIME_TYPE_MANIFEST_LIST: Final[str] = ... | ||
|
|
||
| # TODO: a lot of the properties of this class are ambiguous | ||
| # Mapping[Any] (i.e. Mapping[Any, Any]): can this be narrowed? | ||
AvlWx2014 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| # Perhaps the JsonObject type would be more helpful. | ||
| class ContainerArchiveHelper(object): | ||
| build_image: Mapping[Any] | ||
| build_index: Mapping[Any] | ||
| archive_extra: Mapping[Any] | ||
| archive_docker: Mapping[Any] | ||
| source_tags: Sequence[str] | ||
| arch: str | ||
| labels: Mapping[Any] | ||
| pull_info: ContainerImagePullInfo | ||
|
|
||
| def get_tag_specs(raw_specs: Sequence[str]) -> Sequence[ContainerImagePullSpec_co]: ... | ||
| def get_digest_specs(raw_specs: Sequence[str], digests_map): | ||
| Sequence[ContainerImagePullSpec_co]: ... | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| from collections import Mapping, Sequence, Iterator, Iterable, MutableSequence | ||
| from concurrent.futures import Executor, Future | ||
| from queue import Queue | ||
| from threading import RLock as ReentrantLock | ||
| from types import TracebackType | ||
| from typing import Any, Optional, Union, Type, ClassVar, Final, NoReturn | ||
|
|
||
| from koji import ClientSession, VirtualCall | ||
|
|
||
| from pushsource import Source, RpmPushItem, OperatorManifestPushItem | ||
| from pushsource._impl.model.base import PushItem_co | ||
| from pushsource._impl.type_aliases import MaybeString, JsonObject | ||
|
|
||
| Id = Union[str, int] | ||
AvlWx2014 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| CACHE_LOCK: ReentrantLock | ||
|
|
||
| RETRY_ARGS: Mapping[str, Any] | ||
|
|
||
| class ListArchivesCommand(object): | ||
| def __init__(self, build: Mapping[str, Any]) -> None: ... | ||
| build: Mapping[str, Any] | ||
| # TODO: is VirtualCall correct for this type? | ||
| call: Optional[VirtualCall] = ... | ||
| def execute(self, source: "KojiSource", session: ClientSession) -> int: ... | ||
| def save(self, source: "KojiSource", koji_queue: Queue) -> None: ... | ||
|
|
||
| class GetBuildCommand(object): | ||
| def __init__(self, ident: int, list_archives: bool = ...) -> None: ... | ||
| indent: int | ||
| list_archives: bool = ... | ||
| # TODO: is VirtualCall correct for this type? | ||
| call: Optional[VirtualCall] = ... | ||
| def execute(self, source: "KojiSource", session: ClientSession) -> int: ... | ||
| def save(self, source: "KojiSource", koji_queue: Queue) -> None: ... | ||
|
|
||
| class GetRpmCommand(object): | ||
| def __init__(self, indent: int) -> None: ... | ||
| indent: int | ||
| # TODO: is VirtualCall correct for this type? | ||
| call: Optional[VirtualCall] = ... | ||
| def execute(self, source: "KojiSource", session: ClientSession) -> int: ... | ||
| def save(self, source: "KojiSource", koji_queue: Queue) -> None: ... | ||
|
|
||
| class KojiSource(Source): | ||
| _BATCH_SIZE: Final[ClassVar[int]] | ||
| _koji_session: ClientSession | ||
| def __init__( | ||
| self, | ||
| url: str, | ||
| dest: MaybeString, | ||
| rpm: Optional[Sequence[Id]] = ..., | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if these are right, I think all these arguments can be passed as just plain strings as well? (in which case they'll be split on comma.) Like MaybeString. |
||
| module_build: Optional[Sequence[Id]] = ..., | ||
| module_filter_filename: Optional[Sequence[str]] = ..., | ||
| container_build: Optional[Sequence[Id]] = ..., | ||
| signing_key: Optional[Sequence[str]] = ..., | ||
| basedir: Optional[str] = ..., | ||
| threads: int = ..., | ||
| timeout: int = ..., | ||
| cache: Optional[MutableMapping[str, Any]] = ..., | ||
| executor: Optional[Executor] = ..., | ||
| ) -> None: ... | ||
| def __enter__(self) -> "KojiSource": ... | ||
| def __exit__( | ||
| self, | ||
| exc_type: Type[BaseException], | ||
| exc_val: BaseException, | ||
| exc_tb: TracebackType, | ||
| ) -> None: ... | ||
| def __iter__(self) -> Iterator[PushItem_co]: ... | ||
| # TODO: the return type here feels hacky, but is accurate | ||
| def _koji_check(self) -> None: ... | ||
| def _koji_get_version(self) -> str: ... | ||
| # TODO: reasonably sure based on the Fedora Koji XML-RPC | ||
| # docs for getRPM, getBuild, and getArchive that what is | ||
| # the map returned by those API calls is what's stored in | ||
| # the cache. Since it is not unlike a JSON object I am sticking | ||
| # with a JsonObject return type for now | ||
| def _get_rpm(self, rpm: str) -> JsonObject: ... | ||
| def _get_build(self, build_id: Id) -> JsonObject: ... | ||
| def _get_archives(self, build_id: Id) -> JsonObject: ... | ||
| # TODO: maybe JsonObject is appropriate for the meta parameter type | ||
| def _push_items_from_rpm_meta( | ||
| self, rpm: str, meta: Mapping[str, Any] | ||
| ) -> Sequence[RpmPushItem]: ... | ||
| def _module_filtered(self, file_path: str) -> bool: ... | ||
| def _get_module_name(self, nvr: str, file_path: str) -> str: ... | ||
| # TODO: maybe JsonObject is appropriate for the meta parameter type | ||
| def _push_items_from_module_build( | ||
| self, nvr: str, meta: Mapping[str, Any] | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _push_items_from_container_build( | ||
| self, nvr: str, meta: Mapping[str, Any] | ||
| ) -> Sequence[PushItem_co]: ... | ||
| def _get_operator_item( | ||
| self, nvr: str, meta: Mapping[str, Any], archives: Iterable[Mapping[str, Any]] | ||
| ) -> OperatorManifestPushItem: ... | ||
| def _rpm_futures(self) -> Sequence[Future[PushItem_co]]: ... | ||
| def _modulemd_futures(self) -> Sequence[Future[PushItem_co]]: ... | ||
| def _container_futures(self) -> Sequence[Future[PushItem_co]]: ... | ||
| def _do_fetch(self, koji_queue: Queue, exceptions: MutableSequence) -> None: ... | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| class Module(object): | ||
| name: str | ||
| stream: str | ||
| version: str | ||
| context: str | ||
| arch: str | ||
| nsvca: str | ||
| @classmethod | ||
| def from_file(cls, fname: str) -> "Module": ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| from collections import Iterator | ||
| from re import Pattern | ||
| from types import TracebackType | ||
| from typing import Optional, Type | ||
|
|
||
| from pushsource import Source, PushItem | ||
| from pushsource._impl.model.base import PushItem_co | ||
| from pushsource._impl.type_aliases import MaybeString | ||
|
|
||
| IMAGE_URI_REGEX: Pattern | ||
|
|
||
| class RegistrySource(Source): | ||
| def __init__( | ||
| self, | ||
| image: MaybeString, | ||
| dest: Optional[MaybeString] = ..., | ||
| dest_signing_key: Optional[MaybeString] = ..., | ||
| ) -> None: ... | ||
| def __enter__(self) -> "RegistrySource": ... | ||
| def __exit__( | ||
| self, | ||
| exc_type: Type[BaseException], | ||
| exc_val: BaseException, | ||
| exc_tb: TracebackType, | ||
| ) -> None: ... | ||
| def __iter__(self) -> Iterator[PushItem_co]: ... | ||
| def _push_item_from_registry_uri(self, uri: str, signing_key: str) -> PushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from pushsource import AmiPushItem | ||
| from pushsource._impl.backend.staged.staged_base import StagedBaseMixin | ||
| from pushsource._impl.backend.staged.staged_utils import StagingLeafDir, StagingMetadata | ||
|
|
||
| class StagedAmiMixin(StagedBaseMixin): | ||
| def __push_item( | ||
| self, leafdir: StagingLeafDir, metadata: StagingMetadata, entry: DirEntry | ||
| ) -> AmiPushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| from collections import Callable, Sequence | ||
| from os import DirEntry | ||
| from typing import Final, ClassVar, MutableMapping, Any | ||
|
|
||
| from pushsource import PushItem | ||
| from pushsource._impl.backend.staged.staged_utils import StagingLeafDir, StagingMetadata | ||
| from pushsource._impl.model.base import PushItem_co | ||
|
|
||
| # Typically "self" isn't annotated, but in this case | ||
| # "unbound" methods are being stashed in a dict and | ||
| # the "self" parameter is not fulfilled until later via | ||
| # partial application. This type annotation accounts for | ||
| # the type of unbound "self" parameter in the callable signature. | ||
| UnboundTypeHandlerDelegate = Callable[ | ||
| ["StagedBaseMixin", StagingLeafDir, StagingMetadata, DirEntry], PushItem | ||
| ] | ||
| # A type for an UnboundTypeHandlerDelegate where "self" has | ||
| # been bound to an instance of StagedBaseMixin | ||
| BoundTypeHandlerDelegate = Callable[ | ||
| [StagingLeafDir, StagingMetadata, DirEntry], PushItem | ||
| ] | ||
| # A partially applied type handler callable | ||
| PartialTypeHandler = Callable[[StagingLeafDir, StagingMetadata], Sequence[PushItem_co]] | ||
|
|
||
| class TypeHandler(object): | ||
| HANDLERS: Final[ClassVar[MutableMapping[str, UnboundTypeHandlerDelegate]]] | ||
| type_name: str | ||
| def __init__(self, type_name: str) -> None: ... | ||
| def __call__(self, fn: UnboundTypeHandlerDelegate) -> None: ... | ||
|
|
||
| class StagedBaseMixin(object): | ||
| _FILE_TYPES = Final[ClassVar[MutableMapping[str, PartialTypeHandler]]] | ||
| def __init__(self, *args: Any, **kwargs: Any) -> None: ... | ||
| def __mixin_push_items( | ||
| self, | ||
| leafdir: StagingLeafDir, | ||
| metadata: StagingMetadata, | ||
| delegate: BoundTypeHandlerDelegate, | ||
| ) -> Sequence[PushItem_co]: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from pushsource import CompsXmlPushItem | ||
| from pushsource._impl.backend.staged.staged_base import StagedBaseMixin | ||
| from pushsource._impl.backend.staged.staged_utils import StagingLeafDir, StagingMetadata | ||
|
|
||
| class StagedCompsXmlMixin(StagedBaseMixin): | ||
| def __push_item( | ||
| self, leafdir: StagingLeafDir, _: StagingMetadata, entry: DirEntry | ||
| ) -> CompsXmlPushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from pushsource import ErratumPushItem | ||
| from pushsource._impl.backend.staged.staged_base import StagedBaseMixin | ||
| from pushsource._impl.backend.staged.staged_utils import StagingMetadata, StagingLeafDir | ||
|
|
||
| class StagedErrataMixin(StagedBaseMixin): | ||
| def __make_push_item( | ||
| self, leafdir: StagingLeafDir, metadata: StagingMetadata, entry: DirEntry | ||
| ) -> ErratumPushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from pushsource import FilePushItem | ||
| from pushsource._impl.backend.staged.staged_base import StagedBaseMixin | ||
| from pushsource._impl.backend.staged.staged_utils import StagingLeafDir, StagingMetadata | ||
|
|
||
| class StagedFilesMixin(StagedBaseMixin): | ||
| def __file_push_item( | ||
| self, leafdir: StagingLeafDir, metadata: StagingMetadata, entry: DirEntry | ||
| ) -> FilePushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from pushsource import ModuleMdPushItem | ||
| from pushsource._impl.backend.staged.staged_base import StagedBaseMixin | ||
| from pushsource._impl.backend.staged.staged_utils import StagingLeafDir, StagingMetadata | ||
|
|
||
| class StagedModuleMdMixin(StagedBaseMixin): | ||
| def __push_item( | ||
| self, leafdir: StagingLeafDir, metadata: StagingMetadata, entry: DirEntry | ||
| ) -> ModuleMdPushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from .staged_utils import StagingLeafDir, StagingMetadata | ||
| from ...model import ProductIdPushItem | ||
| from .staged_base import StagedBaseMixin | ||
|
|
||
| class StagedProductIdMixin(StagedBaseMixin): | ||
| def __push_item( | ||
| self, leafdir: StagingLeafDir, _: StagingMetadata, entry: DirEntry | ||
| ) -> ProductIdPushItem: ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from os import DirEntry | ||
|
|
||
| from .staged_base import StagedBaseMixin | ||
| from .staged_utils import StagingLeafDir, StagingMetadata | ||
| from ...model import RpmPushItem | ||
|
|
||
| class StagedRpmMixin(StagedBaseMixin): | ||
| def __push_item( | ||
| self, leafdir: StagingLeafDir, _: StagingMetadata, entry: DirEntry | ||
| ) -> RpmPushItem: ... |


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'm not sure this is right for a couple of reasons:
I could easily be mistaken here since co/contravariance is something I always have to look up again every time it arises.
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 also have to do some reading every time I encounter variance. I always come away thinking I have a better handle on it, and inevitably prove myself wrong the next time. That said, I definitely made some mistakes throughout that I need to correct.
The main case is related to your second bullet point. Given that immutable collection types are already declared covariant in their type parameter I can safely drop a number of
*_cotype variable usages throughout.What I wanted to ensure by using both
boundandcovariantis that the type variable is not only covariant, but covariant with a specific upper bound (ErrataRawin this case, butPushItem_cois quite ubiquitous in what I've done so far). I definitely need to do some more reading about it. Maybe I can construct a solid example that demonstrates a case where both are needed.