Modernize type annotations and fix some discrepancies #451
Conversation
* Clarify that each SDO object has its corresponding OD entry linked in the .od attribute, not the whole ObjectDictionary. * __iter__() should return Iterator like in Mapping, not Iterable. * Use postponed evaluation of type annotations (PEP-563 / PEP-649 style) instead of stringized type annotations.
|
This doesn't include the fixes from #446, but goes in a similar direction. Could merge those cleanup-only changes into this PR though if you agree @friederschueler. |
Use postponed evaluation of type annotations (PEP-563 / PEP-649 style) instead of stringized type annotations.
PdoVariable.__getitem__() is annotated to not return None. In fact, every branch sets a valid PdoVariable return value, or raises an exception. Therefore the None default value is unnecessary and trips the type checker (mypy).
Use postponed evaluation of type annotations (PEP-563 / PEP-649 style) instead of stringized type annotations. Conditionally import types from other sub-packages to avoid dependency cycles.
|
Integrated the relevant changes from #446 here. This is certainly not complete yet, for all of the code-base. But better modernize a tiny bit than never. |
|
@friederschueler, @sveinse would you mind reviewing this change in total? Would like to get it into the next release, which we should make soon because of the breakage mentioned in #455. |
| def __init__(self, node: Union[LocalNode, RemoteNode]): | ||
| self.network: Optional[Network] = None | ||
| self.map: Optional[PdoMaps] = None | ||
| self.node: Union[LocalNode, RemoteNode] = node |
There was a problem hiding this comment.
What is the intention of having Union[LocalNode, RemoteNode] as just using BaseNode? Do we expect other BaseNode derivatives that are not compatible with PdoBase
There was a problem hiding this comment.
Simply because we need the .sdo attribute here and that is not part of BaseNode. I previously asked the same question in #446 (comment).
There was a problem hiding this comment.
Yes, otherwise we only have network, object_dictionary and id attributes. Also explicit is better than implicit, thats why I don't like defining these attributes in the BaseNode which would be another solution.
There was a problem hiding this comment.
Great, thanks.
Perhaps we should at some point make a NodeProtocol for the required attributes instead of using a exhaustive list? It might not be worth the efforts thou, since the Union is only used a few places.
There was a problem hiding this comment.
Agreed, I think the Union approach is simple enough and warranted. We can still rework it when there's a stronger argument for it.
| @@ -30,7 +31,7 @@ def __init__( | |||
| self, | |||
| rx_cobid: int, | |||
| tx_cobid: int, | |||
| od: ObjectDictionary, | |||
| od: objectdictionary.ObjectDictionary, | |||
There was a problem hiding this comment.
I have read somewhere that using modules for reference to type hints should be avoided. But I've tested the following code with 3.8 and it seems to work. So perhaps I'm misremembering.
import canopen
def foo(a: canopen.Node):
passThere was a problem hiding this comment.
Well if you do find such a recommendation and it has a valid reason, then we can still change that later. I don't see any reason why we shouldn't do it and as it seems to work (at least mypy doesn't complain here), let's try.
acolomb
changed the title
Clarify that each SDO object has its corresponding OD entry linked in the
.odattribute, not the wholeObjectDictionary.__iter__()should returnIteratorlike inMapping, notIterable.Use postponed evaluation of type annotations (PEP-563 / PEP-649 style) instead of stringized type annotations.