Repository navigation
Conversation
There was a problem hiding this comment.
I'm not super enthusiastic about undoing the generic stuff to work around what seems to be PyCharm bug, although quoting the type hinting is a great improvement over the type vars. I'll try this out a bit more to see if the type hinting works everywhere as intended, or if the auto-complete sometimes only shows e.g. BaseResourceModel. Some of the template stuff can be cleaned up I think.
| {%- if used_models -%}( | ||
| {%- for name in used_models -%} | ||
| Generic[T{{ name }}]{%- if not loop.last -%}, {%- endif -%} | ||
| "{{ name }}"{%- if not loop.last -%}, {%- endif -%} |
There was a problem hiding this comment.
since your code no longer uses the class_model_bindings or typevar_model_bindings macros, can't you just delete them?
| def resource_name_suffix(name): | ||
| # add a suffix to prevent typing conflicts | ||
| if name != "ResourceModel": | ||
| return f"{name}ResourceModel" |
There was a problem hiding this comment.
there's only one resource model, so this is confusing. it's also pretty heavyweight. maybe something like we do in the Java plugin in a simpler solution? (models already are guaranteed to have unique names, so you only have to disambiguate clashes with languages types. as far as i can see, the same is true for type hints)
There was a problem hiding this comment.
ah, I see why you may have done this. interesting mypy errors happen when a variable has the same name as a type (the variable shadows the type for type-hints only)
|
|
||
|
|
||
| def set_or_none(value: Optional[Sequence[T]]) -> Optional[AbstractSet[T]]: | ||
| def set_or_none(value: Optional[Sequence[Any]]) -> Optional[AbstractSet[Any]]: |
There was a problem hiding this comment.
if you keep T = TypeVar("T"), you can leave this as T and provide stronger type guarantees
|
|
||
| @dataclass | ||
| class {{ model }}{{ class_model_bindings(properties) }}: | ||
| class {{ model|resource_name_suffix }}(BaseResourceModel): |
There was a problem hiding this comment.
sub-models aren't and shouldn't be valid instances of BaseResourceModel, so only the actual resource model should inherit from BaseResourceModel, right?
|
closing in favor of #51 |
|
Depending on the reasons to keep the type hinting and the behaviour of mypy, there might be a way to use generics and still have typehinting work in pycharm. It's not super clean, but might be workable: Generating does make pycharm work in that method. The big downside of going back to generics, is that that same ide still complains about the [] syntax in the type hints:
|
|
oh, interesting! from my side, i'm happy with the approach Jay suggested. coming into a new code-base is always tough, especially with dynamic languages. so the auto-completion really helps getting resource authors going. (if someone accidentally deleted that line, i'm guessing it would break again?) |
|
It would break indeed. It is basically telling pycharm/mypy/... "I know better, use this as the type". |
3 participants
Issue #, if available:
Description of changes:
Main aim of this pr is to simplify the representation of type hints in the handler and improve IDE resolution of types for inline hinting/validation.
A notable sacrifice was that I had to set the type for the
ResourceHandlerRequestinHandlerSignaturetoAnyto get mypy to accept that the actual handler was usingResourceHandlerRequestnotBaseResourceHandlerRequest.Previously PyCharm (didn't test other IDE's) was not able to resolve types for
ResourceModelorProgressEvent. Type completion/validation now works as expected.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.