Add type information to pushsource #207
Conversation
There are a number of model class which are just "empty" subclasses of other models (e.g. ProductIdPushItem(PushItem)), which don't define any additional properties or methods outside of those provided by their base class. It may not be necessary (not 100% sure yet) to stub such classes, but it does provide a place where future stubs will go when needed.
|
@rohanpm - thought a Draft PR would be a better place to discuss this than GChat. Not sure what is up with the |
| ErrataRaw_co = TypeVar("ErrataRaw_co", bound="ErrataRaw", covariant=True) | ||
|
|
||
| class ErrataRaw(object): | ||
| advisory_cdn_metadata: JsonObject |
There was a problem hiding this comment.
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?
| _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: ... |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm curious does it cause problems if some of these are left out?
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 _log_queried_et if I take it out of the stub file (see first screenshot), and flags imports of names missing from stubs as an error (see second screenshot). The severity of these happens to be configurable in PyCharm, but that may not be the case in all IDEs.
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 don't want to require that type hints must be added even for private methods on private classes
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.
|
|
||
| from pushsource.type_aliases import JsonObject | ||
|
|
||
| ErrataRaw_co = TypeVar("ErrataRaw_co", bound="ErrataRaw", covariant=True) |
There was a problem hiding this comment.
I'm not sure this is right for a couple of reasons:
- is it right to mix 'bound' and 'covariant'? Docs for TypeVar say "Type variables may be marked covariant or contravariant by passing covariant=True or contravariant=True. [...] Alternatively, a type variable may specify an upper bound using bound=". "Alternatively" should mean picking one or the other. I guess covariant is redundant if you use bound?
- PEP484 also says "The read-only collection classes in typing are all declared covariant in their type variable (e.g. Mapping and Sequence)", and this is only used in Sequence, so maybe it's just redundant altogether?
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.
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 *_co type variable usages throughout.
What I wanted to ensure by using both bound and covariant is that the type variable is not only covariant, but covariant with a specific upper bound (ErrataRaw in this case, but PushItem_co is 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.
| @@ -0,0 +1,32 @@ | |||
| from collections import Sequence | |||
| from typing import Text, Optional | |||
There was a problem hiding this comment.
I'm not sure about the usage of Text here because it immediately makes me think "which of the other str cases also should be Text" ?
I would be in favor of saying that the type hints are just meant to be for py3, in which case Text is the same thing as str, and consistently use str throughout. I'd doubt that the Text vs str split we see currently is accurately reflecting which fields might receive unicode objects on py2.
There was a problem hiding this comment.
You're right. I started with the classes in model/ and since then talked myself out of using Text so we're not forcing people to worry about unicode in Python 2 instead of plain strings. I have a note to myself to go back to the classes in model/ and strip out uses of Text as well as add stubs for dunders, and I have just started on that recently.
|
|
||
| from pushsource._impl.model.base import PushItem_co | ||
|
|
||
| SourceFactory = Callable[[], "Source"] |
There was a problem hiding this comment.
I would rather not require that a SourceFactory has to return a type of Source or a subclass. At the moment, you can actually return any iterable of push items and some tests take advantage of this, because it's an easy way to set up a test if you just have a hardcoded list of items you want to return.
There was a problem hiding this comment.
How about this instead (incorporating the fact that immutable Iterable is covariant and thus doesn't need PushItem_co):
PushItemIterator = Callable[[], Iterable[PushItem]]
src/pushsource/type_aliases.pyi
Outdated
|
|
||
| MaybeString = Union[str, Collection[str]] | ||
|
|
||
| # First attempt at a JsonObject type based on the spec from |
There was a problem hiding this comment.
I commented elsewhere as well, but repeating here. Where this class is used, the underlying APIs are not actually JSON (both ET and koji are being interacted with using xml-rpc). Although they are pretty close and the type hints might work, it's a bit wrong to be following the JSON spec to come up with the types when there's no JSON in sight.
Elsewhere I said ApiObject, but maybe the top-level type should really be something like Struct or XmlRpcStruct. I guess what the type is modelling is precisely those types in the "XML-RPC type" table at https://docs.python.org/3/library/xmlrpc.client.html .
Fix copy/paste error in __enter__ stub. Co-authored-by: Rohan McGovern <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #207 +/- ##
=============================================
+ Coverage 0 100.00% +100.00%
=============================================
Files 0 43 +43
Lines 0 1749 +1749
=============================================
+ Hits 0 1749 +1749
Continue to review full report at Codecov.
|
|
About one year on from the original draft here, this repo has now dropped python 2 support, so it would be possible to start using type hints throughout the codebase without having to use the .pyi stubs. |
Overview
This PR is a work in progress, but I wanted to have a place for ongoing discussion that can be referenced later.
Motivation
Enhance developer experience by adding typing information to
pushsource. Many popular IDEs like PyCharm CE and VSCode have built-in support for Python type annotations (e.g. built-in type checkers). Packaging newer code with type information allows developers that depend on (or maintain) such code to leverage those IDE tools for a nicer experience.Approach
Since Python < 3.5 does not have syntax support for type annotations there were two main ways to go about this. One way is through the use of both inline (e.g.
# type: str)and function (e.g.# type: (str, str) -> bool) throughout the code as suggested in one part of PEP 484. The other is stub files, which are also outlined in PEP 484. I chose the latter since I think it's neater (doesn't clutter the code), and because of the wealth of examples from the standard library (through the typeshed project) and well-maintained projects like NumPy.The addition of the
py.typedfile, as well as a declaration of that file insetup.pyis recommended in PEP 561 for distributing stub files that are located alongside their associated source files.Useful Resources