Support string-encoded types as per PEP-0563 #39
Conversation
ftomassetti
left a comment
ftomassetti
left a comment
There was a problem hiding this comment.
I added minor comments but my competence with Python is very limited, so please feel free to ignore them if they are irrelevant or wrong
| return isinstance(decl_type, type) and issubclass(decl_type, Node) | ||
|
|
||
|
|
||
| def get_only_type_arg(decl_type): |
There was a problem hiding this comment.
Would it make sense to use type annotations for both the parameter and the return type?
There was a problem hiding this comment.
I wouldn't know how to properly type them
|
|
||
| def get_only_type_arg(decl_type): | ||
| type_args = get_type_arguments(decl_type) | ||
| if len(type_args) == 1: |
There was a problem hiding this comment.
If the type_args are more than one should we throw an exception perhaps?
If not, I would add a short document to explain it
| return None | ||
|
|
||
|
|
||
| def process_annotated_property(name, decl_type, known_property_names): |
There was a problem hiding this comment.
Also here I would consider adding type annotations
| def process_annotated_property(name, decl_type, known_property_names): | ||
| multiplicity = Multiplicity.SINGULAR | ||
| is_reference = False | ||
| if get_type_origin(decl_type) is ReferenceByName: |
There was a problem hiding this comment.
In Python I often get confused by is because I should use isinstance instead. I am not sure if that is the case here, but I mention it to be on the safe side
There was a problem hiding this comment.
is is Java's ==
== is .equals
isinstance is instanceof
Here, we want to check if the type is exactly ReferenceByName
pylasu/model/model.py
Outdated
| @internal_property | ||
| def properties(self): | ||
| return (PropertyDescription(p.name, p.provides_nodes, p.multiplicity, getattr(self, p.name)) | ||
| return (PropertyDescription(p.name, p.type, p.is_containment, p.is_reference, p.multiplicity, |
There was a problem hiding this comment.
when there are many parameters and several of them are boolean, we could pass them by name to avoid possible issues (i.e., writing them in the wrong order)
| else: | ||
| return getattr(cls, '__annotations__', None) | ||
| try: | ||
| # On Python 3.10+ |
There was a problem hiding this comment.
Should we mention this in the README?
There was a problem hiding this comment.
Mention what? The algorithm for getting the annotations?
Fixes #38