typing does not (often) match doctrings

[jvanasco]

While upgrading a project that depends on Certbot, I encountered a handful of typing errors.

One such example is in crypto_utils.get_names_from_cert https://github.com/certbot/certbot/blob/master/certbot/certbot/crypto_util.py#L459-L470

The cert param is typed for bytes, but the doctring - and former usage - is typed for string.

Multiple functions in this file have similar inconsistencies.

[alexzorin]

The types are checked by mypy, they are the accurate ones.

We should find an automated refactoring method to either ensure that the docstrings are correct, or update the docstrings so that they no longer include type information, since it is redundant.

[jvanasco]

The types are checked by mypy, they are the accurate ones.

Are you sure? Many (all?) of these methods accept both str and bytes.

I was trying to figure out if there was a conscious decision to backport compatibility, or if these functions were intended to accept Union[bytes,str], but that got lost when adding type hints to certain functions.

I wasn't able to find anything in terms of a styleguide in the repo or tickets.

Please do not interpret this comment as "challenging your decision", but as a suggestion that it may be prudent to take a pause and ensure this is desired.

[alexzorin]

The types are accurate in the sense that they are automatically checked by mypy. The docstrings are just text and I know that they are wrong in some places.

Some functions could potentially accept broader types, as you note. That is something that has to be decided on a case-by-case basis and seeing what makes sense for API design.

I recall we did "break" some API compatibility while adding the types. In some cases, this was unavoidable, because the docstring types were truly wrong and the only way to make mypy happy was to correct the types so that they made sense.

As far as style guide goes, from reviewing the typing PRs, I recall that we just followed the advice in the typing module, e.g.:

Note that to annotate arguments, it is preferred to use an abstract collection type such as Mapping rather than to use dict or typing.Dict.

[chris-zenfolio]

I have read through the issue and seem a bit confused. I understand using mypy for the type checking (as I also use mypy to check my code), but I don't understand why a resource would not be assigned to update the docstrings to ensure users both internal and external to Cloudflare would have accurate documentation on how to use the Python methods. I already struggle to figure out what type of an object is going to be returned. Having that kind of information would be beneficial with implementing type hints. This could be put in the docstrings, if they are updated.