ENT-5106: Type-hint subscription_manager/ files#3173
Conversation
|
#3181 needs to be merged first, as it resolves issues with writing to wrong locations. |
acc276e to
bd503f3
Compare
ptoscano
left a comment
There was a problem hiding this comment.
Ooof, lot to go through and check (with associated sadness due to the discovery of horrors done in the past)
I left various notes with things to fix in this PR, and things which would be better to fix outside of this (possibly even before this, simplifying things a bit).
Also, all around both Dict and dict are used: even if Dict is deprecated in Python 3.9, I think it'd be better to use Dict all around, so a) there is consistency b) new code that uses Dict can be easily backported to 1.28 if needed.
src/subscription_manager/cli.py
Outdated
| # taken wholseale from rho... | ||
| class CLI(object): | ||
| def __init__(self, command_classes=None): | ||
| def __init__(self, command_classes: List[AbstractCLICommand] = None): |
There was a problem hiding this comment.
i think this should be List[Type[AbstractCLICommand]], since it is a list of class types and not actual instances
src/subscription_manager/cache.py
Outdated
| # FIXME This return signature feels strange | ||
| def get_overall_status_code(self) -> Union[Dict, str]: |
There was a problem hiding this comment.
more than strange, it seems wrong to me: the only place that calls this is the "status" command, and it is only compared against a static string:
subscription-manager/src/subscription_manager/cli_command/status.py
Lines 147 to 152 in d992540
which means that, if the server status is valid, the above check will be always false; then IMHO simply because there are no reasons (
get_status_reasons() => None) when the status is valid and matched, then nothing happens to be printed
IMHO this ought to return server_status["status"], which is the status string -- something to change in a followup change
src/subscription_manager/cache.py
Outdated
| try: | ||
| f = open(self.CACHE_FILE) | ||
| data = self._load_data(f) | ||
| data: str = self._load_data(f) |
There was a problem hiding this comment.
hmm it seems to me that _load_data() returns Optional[Dict], not str -- at least, reading all the implementation of this method in all the cache subclasses
src/subscription_manager/cpuinfo.py
Outdated
|
|
||
| @classmethod | ||
| def from_uname_machine(cls, uname_machine, prefix=None): | ||
| def from_uname_machine(cls, uname_machine: str, prefix: str = None) -> BaseCpuInfo: |
There was a problem hiding this comment.
maybe Optional[str] instead of str?
There was a problem hiding this comment.
Yeah, both mypy and PyCharm's type checker can convert this implicitly, when they see it being assigned to None, that's why it was not here. Updated per your suggestion for clarity.
src/subscription_manager/cpuinfo.py
Outdated
| @classmethod | ||
| def open_proc_cpuinfo(cls, prefix=None): | ||
| proc_cpuinfo_path = cls.proc_cpuinfo_path | ||
| def open_proc_cpuinfo(cls, prefix: str = None) -> str: |
There was a problem hiding this comment.
maybe Optional[str] instead of str?
src/subscription_manager/repofile.py
Outdated
| self.gpgkey_ssl_verify: Optional = None | ||
| self.repo_ssl_verify: Optional = None |
There was a problem hiding this comment.
Optional[str] for both, from what I can see
src/subscription_manager/repolib.py
Outdated
| release_source = YumReleaseverSource() | ||
|
|
||
| # query whether OCSP stapling is advertized by CP for the repositories | ||
| # query whether OCSP stapling is advertised by CP for the repositories |
There was a problem hiding this comment.
strictly speaking, "advertized" is correct (American English)
There was a problem hiding this comment.
I must have misconfigured the spellcheck.
| """ | ||
|
|
||
| def __init__(self, product=None): | ||
| def __init__(self, product: "Product" = None): |
There was a problem hiding this comment.
maybe Optional["Product"]?
src/subscription_manager/utils.py
Outdated
| """ | ||
| if not sys.stdout.isatty(): | ||
| return None | ||
| # FIXME fallback should be integers |
There was a problem hiding this comment.
I used the non-integer fallback on purpose: this was the code can know when get_terminal_size() failed and returned the fallback, so the 1000 value can be used
src/subscription_manager/utils.py
Outdated
|
|
||
| class EntitlementCertificateFilter(ProductCertificateFilter): | ||
| def __init__(self, filter_string=None, service_level=None): | ||
| def __init__(self, filter_string: str = None, service_level=None): |
There was a problem hiding this comment.
i guess Optional[str] for filter_string? also, did you forget service_level here? :)
m-horky
left a comment
There was a problem hiding this comment.
both
Dictanddictare used
That is true, but only when it is not further described: only List[dict], never dict[str, str]. That still makes it 3.6-compatible. I think I was quite consistent, but it may have slipped here and there. I only found one occasion of dict[] in cloud_facts.py, which I have fixed now.
src/subscription_manager/cpuinfo.py
Outdated
|
|
||
| @classmethod | ||
| def from_uname_machine(cls, uname_machine, prefix=None): | ||
| def from_uname_machine(cls, uname_machine: str, prefix: str = None) -> BaseCpuInfo: |
There was a problem hiding this comment.
Yeah, both mypy and PyCharm's type checker can convert this implicitly, when they see it being assigned to None, that's why it was not here. Updated per your suggestion for clarity.
| return message | ||
|
|
||
| def format_using_error(self, exc: Exception, _: str) -> str: | ||
| def format_using_error(self, exc: Exception, _: Any) -> str: |
There was a problem hiding this comment.
This function is only called once (in this file, in get_message) with the argument being None.
src/subscription_manager/identity.py
Outdated
| # TODO: we're using a Certificate which has it's own write/delete, no idea | ||
| # why this landed in a parallel disjoint class wrapping the actual cert. | ||
| def write(self): | ||
| # why this landed in a parallel disjoint class wrapping the actual cert. |
There was a problem hiding this comment.
When there are at least two spaces after the hash sign, PyCharm renders all the comment lines as TODO comment, not just the first line.
But we are not consistent in this (just in the 20 lines above there are both indented and non-indented follow-up lines). Reverting.
|
|
||
| def main(self): | ||
| cmd = self._find_best_match(sys.argv) | ||
| def main(self) -> Optional[int]: |
There was a problem hiding this comment.
Updated with a FIXME.
| # FIXME Does not seem to be used | ||
| def fetch_certificates(certlib) -> Literal[True]: |
There was a problem hiding this comment.
I'd rather fix this after merging, to prevent further delays and rebases. There will be many minor things to fix, this is just one more to the list.
| # dict which does not contain all the pool info. Not sure if this is really | ||
| # necessary. Also some "view" specific things going on in here. |
There was a problem hiding this comment.
Same as above. Reverting :(
| if not isinstance(name, str): | ||
| # FIXME This is not necessary in Python 3 code | ||
| name = name.decode("utf-8") |
There was a problem hiding this comment.
Same as with the other ones, the PR should be a follow-up.
src/subscription_manager/repolib.py
Outdated
| release_source = YumReleaseverSource() | ||
|
|
||
| # query whether OCSP stapling is advertized by CP for the repositories | ||
| # query whether OCSP stapling is advertised by CP for the repositories |
There was a problem hiding this comment.
I must have misconfigured the spellcheck.
src/subscription_manager/utils.py
Outdated
| """ | ||
| if not sys.stdout.isatty(): | ||
| return None | ||
| # FIXME fallback should be integers |
3c8ba89 to
bbf605b
Compare
jirihnidek
left a comment
There was a problem hiding this comment.
Thanks for this PR. 👍 I have few small requests. Next time please do not create so big PRs, when it is not necessary. Thanks.
| @@ -12,71 +12,78 @@ | |||
| # granted to use or replicate Red Hat trademarks that are incorporated | |||
| # in this software or its documentation. | |||
| # | |||
There was a problem hiding this comment.
Please add at least empty line between GPL license and first line of code.
src/subscription_manager/plugins.py
Outdated
|
|
||
| # clazz is the class object for class instance of the object the hook method maps too | ||
| def __init__(self, clazz, conf=None): | ||
| def __init__(self, clazz: type(SubManPlugin), conf: Optional["PluginConfig"] = None): |
There was a problem hiding this comment.
Wouldn't be better to use Type[SubManPlugin]? Calling type() in type hint looks wild.
src/subscription_manager/plugins.py
Outdated
| slots = ["pre_register_consumer"] | ||
|
|
||
| def __init__(self, clazz, name, facts): | ||
| def __init__(self, clazz: type(SubManPlugin), name: str, facts: Dict[str, str]): |
src/subscription_manager/plugins.py
Outdated
| slots = ["pre_product_id_install", "post_product_id_install"] | ||
|
|
||
| def __init__(self, clazz, product_list): | ||
| def __init__(self, clazz: type(SubManPlugin), product_list: List[dict]): |
src/subscription_manager/plugins.py
Outdated
| slots = ["pre_product_id_update", "post_product_id_update"] | ||
|
|
||
| def __init__(self, clazz, product_list): | ||
| def __init__(self, clazz: type(SubManPlugin), product_list: List[dict]): |
There was a problem hiding this comment.
And here and some other places...
| remote: dict = None, | ||
| base: dict = None, | ||
| uep: "UEPConnection" = None, | ||
| consumer_uuid: str = None, |
There was a problem hiding this comment.
All these arguments should be Optional.
src/subscription_manager/utils.py
Outdated
|
|
||
|
|
||
| def is_simple_content_access(uep=None, identity=None): | ||
| def is_simple_content_access(uep: "UEPConnection" = None, identity: Optional["Identity"] = None) -> bool: |
There was a problem hiding this comment.
The uep should be also Optional["UEPConnection"]
src/subscription_manager/utils.py
Outdated
|
|
||
|
|
||
| def get_current_owner(uep=None, identity=None): | ||
| def get_current_owner(uep: "UEPConnection" = None, identity: "Identity" = None) -> dict: |
38fe2b7 to
e3272f6
Compare
jirihnidek
left a comment
There was a problem hiding this comment.
Thanks for updates. 👍 LGTM
ptoscano
left a comment
There was a problem hiding this comment.
mostly LGTM now; I left a couple of easy notes, and please notice an unfixed note (still about Optional)
also please rebase it, so we can get updates and CI improvements
| self.system_status = None | ||
| self.valid_entitlement_certs = None | ||
| self.status = None | ||
| def __init__(self, on_date: datetime = None): |
| self.on_date: datetime = on_date | ||
| self.installed_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.unentitled_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.expired_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.partially_valid_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.valid_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.partial_stacks: Dict[str, List[EntitlementCertificate]] = None | ||
| self.future_products: Dict[str, List[EntitlementCertificate]] = None | ||
| self.reasons: Reasons = None | ||
| self.supports_reasons: bool = False | ||
| self.system_status: str = None | ||
| self.valid_entitlement_certs: List[EntitlementCertificate] = None |
There was a problem hiding this comment.
all of them (except the bool one) should be Optional[...]
e3272f6 to
3b5766f
Compare
ptoscano
left a comment
There was a problem hiding this comment.
Jirka approved, and now I do it too.
Will wait for the successful completion of the CI jobs before merging.
|
The |
* Card ID: ENT-5106 - Along with type hints, some typos in comments have been fixed and some import statements were reordered.
3b5766f to
980ae50
Compare
Unfortunately, it is a large PR due to the card scope. Most of the files should be easy to review though, as frequently there were only some minor changes.
Because type hints of function arguments and return values are evaluated at runtime, some have been put into quotes as strings -- they are not being loaded, they are imported only when
TYPE_CHECKINGis True. Variable arguments are not evaluated at runtime, so they do not need to be stringified.