Add Python type hints to generated model code #50
Conversation
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
It is now defined yet at this stage Signed-off-by: Arthit Suriyawongkul <[email protected]>
Coverage report
This PR does not seem to contain any modification to coverable code. |
Use `typing.Set` instead of built-in `set`, for better backward compatibility. - type hint with built-in `set` is supported since 3.9 inclusive - type hint with typing.Set is supported back to 3.5 inclusive Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Use Dict instead of dict for type hinting Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
@bact I don't know much about type hinting; is it possible to verify the hints when running CI tests? |
Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
I have tried https://pyrefly.org/ and it was boooomm. One thing is it detects that the superclass of I have tried to changed it from Running Btw, the generated code after the Pyrefly "fixed" is no longer pass the CI test (it was before). So not a good sign. |
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Implement reviewer's suggestion Signed-off-by: Arthit Suriyawongkul <[email protected]> Co-Authored-By: Joshua Watt <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
This should make it pass the pytest Signed-off-by: Arthit Suriyawongkul <[email protected]>
1) Use Collection from typing instead of from collections.abc To keep compatibility with Python 3.8 2) Add comment for "# ignore[arg-type]" at f.write(chunk) in JSONLDSerializer.write() method .write() expects either str or bytes, depends on how file is open. But the type checker wants it to be explicit here which one we like to have. Since chunk is bytes and we open it in "wb" (binary) mode, this is already correct. So we tell the type checker to ignore this line. Add this notes to the comment of JSONLDSerializer.write() Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
Pass all pytests now. It failed also because I used an annotation that is not available in Python 3.8 -- fixed. |
Signed-off-by: Arthit Suriyawongkul <[email protected]>
No issues from mypy now Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
A workflow to test the generated Python binding is added. |
|
I'd prefer here. Thanks |
|
Would it be possible to add the checker as a dev dependency and then run it in pytest (sort of like the whitespace check)? I think that would be more convenient since it needs to run on the generated code |
Fix all pyright issues Signed-off-by: Arthit Suriyawongkul <[email protected]>
Add class TestCheckType to test mypy, pyrefly, and pyright from pytest Signed-off-by: Arthit Suriyawongkul <[email protected]>
Use ones in pytest instead Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
Latest version for Python 3.8 is 1.14.1 Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
The static type checks for the generated Python binding code are added to pytest in test_python.py. Dev dependency in pyproject.toml is updated. All tests are passed. |
|
Cool thanks. Sees like a good change, but I'll take a more careful look when I have some time |
context=[] is now context=None as default argument. Then in the method, if context is None, set it to []. Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
There was a problem hiding this comment.
In general the type annotations seem useful. I'm not sure about the many additional isinstance() type checks that effectively (silently) bypass parts of the code if the type is what not it expected. This seems arguably "worse" than not doing the type checks since it will probably cause unexpected strange errors in other places in the code if the wrong types are used, as opposed to failing at the location of the wrong type. It feels like those checks are primarily there to make the checkers happy, but I don't think they actually make the code better. This surprises me because I would assume the type annotations would make the code "better" at dealing with types by allowing it to be statically analyzed, not worse.
With type hints for self.objects there's no longer a need to do "if o is None" check. With cast(), there's no longer a need for indirect assignment and to do isinstance() check. Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
I will now try to remove some of the isinstance checks and None checks that I have added. Looks like after we have enough type hints, some of these are no longer needed. Thank you for the detailed review. |
These checks were introduced before the variables got type hints. Also applied suggestions in tests. Signed-off-by: Arthit Suriyawongkul <[email protected]>
Was thought necessary, but it's not Signed-off-by: Arthit Suriyawongkul <[email protected]>
In our code, write_list_item() will always be called under write_list() context, which will guaranteed that self.data is a list (if not None) Signed-off-by: Arthit Suriyawongkul <[email protected]>
Within write_object() context, through _encode_properties(), it guarantees self.data is a dict (if not None). In our code, write_property() always called under the write_object() context. Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
_id already have a type hint, this is no longer necessary Signed-off-by: Arthit Suriyawongkul <[email protected]>
With type hint, it is not necessary Signed-off-by: Arthit Suriyawongkul <[email protected]>
Assign any string here to ensure visibility at class level, _OBJ_TYPE will get updated again by the @register decorator Signed-off-by: Arthit Suriyawongkul <[email protected]>
It is either str or None Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
2 participants
Uh oh!
There was an error while loading. Please reload this page.