Add typing to component init #2276
Conversation
|
Hi - this is really, really cool. I would love to see this landed into Dash, and if I can help with reviewing this PR, debugging any tests, etc., please let me know. I was working on a separate effort to generate stub (.pyi) files, so I have tinkered around with this part of the codebase a little bit. I'm excited to see where this goes! |
| optionalArray: typing.List = Component.UNDEFINED, | ||
| optionalBool: bool = Component.UNDEFINED, | ||
| optionalFunc: typing.Any = Component.UNDEFINED, | ||
| optionalNumber: typing.Union[float, int] = Component.UNDEFINED, |
There was a problem hiding this comment.
@alexcjohnson @T4rk1n just wondering if people can pass in Decimals in here as well?
There was a problem hiding this comment.
If to_json_plotly can serialize it then yes. It's pretty flexible, so probably Decimal is fine, but I haven't tested it.
There was a problem hiding this comment.
Decimal can serialize, I think we can put numbers.Number to get all the number types.
There was a problem hiding this comment.
numbers.Number is too much, it includes complex - how about numbers.Real?
There was a problem hiding this comment.
https://docs.python.org/3/library/numbers.html#the-numeric-tower
Number is the base class, then they extend the last one, to get the Integral you need all the other 3 implemented. Tested with numpy and Number is what get the most support and our to_json supports them, I dont think custom implementation of those works.
There was a problem hiding this comment.
hmph ok, numbers.Real doesn't catch Decimal for some reason, but numbers.Number does. So OK, we can leave it as you have it (int, float, numbers.Number) - better to allow some invalid items through than to prevent some valid items.
| def __init__( | ||
| self, | ||
| children: typing.Union[str, int, float, Component, typing.List[typing.Union[str, int, float, Component]]] = None, | ||
| optionalArray: typing.List = Component.UNDEFINED, |
There was a problem hiding this comment.
this should be a List[Any]. mypy will throw an error on this.
also, wondering if we should use the parent type Iterable.
There was a problem hiding this comment.
typing.List is same as typing.List[typing.Any], iterable might not json compatible, I think the List is the most safe for this typing.
There was a problem hiding this comment.
There are certainly a number of other iterables people frequently use though - tuple, range, dict_keys, dict_values, etc etc... I wouldn't want to push people into casting everything to a list, that would be unnecessary overhead.
There was a problem hiding this comment.
range, dict_keys, dict_values,
Those already needs to be cast to list for json.
I guess we could do a union of Tuple and List.
There was a problem hiding this comment.
We can add Tuple as union for untyped arrays, but arrayOf is really a different typing on both side.
There was a problem hiding this comment.
Thanks for updating array. Not sure what you mean by "arrayOf is really a different typing on both side", but I think it's fairly important to accept tuples in arrayOf. I recall for example various database drivers returning query results as either nested tuples or tuples of dicts.
I wonder if it would be worth defining up front a couple of extra types we can reuse later, like:
JsonNumber = typing.Union[int, float, numbers.Real] # or whatever we end up with for numbers
JsonArray = typing.Union[typing.List, typing.Tuple]
ReactNode = typing.Union[
typing.Union[str, JsonNumber, Component], # note more numbers here!
JsonArray[typing.Union[str, JsonNumber, Component]]
]If we did something like that, would we be able to change:
optionalArrayOf: typing.List[typing.Union[int, float, numbers.Number]] = Component.UNDEFINEDinto this?
optionalArrayOf: JsonArray[JsonNumber] = Component.UNDEFINEDThere was a problem hiding this comment.
It's better to go without defining type aliases that would require to be imported and would break backward compatibility for component libraries that compiled with the new typing but the user has dash locked to a previous version.
|
I went with The checker and my IDE don't recognize the type of |
|
@alexcjohnson can you please review this for us? thank you - @gvwilson |
dash/development/_py_prop_typing.py
Outdated
| typed = get_prop_typing( | ||
| type_info["value"]["name"], component_name, prop_name, type_info["value"] | ||
| ) | ||
| return f"typing.Union[typing.Sequence[{typed}], typing.Tuple]" |
There was a problem hiding this comment.
Why do we need the Union here? Sequence includes tuples, I believe?
There was a problem hiding this comment.
Good catch, I changed from List but didn't remove the Tuple.
dash/development/_py_prop_typing.py
Outdated
|
|
||
|
|
||
| PROP_TYPING = { | ||
| "array": generate_type("typing.Union[typing.Sequence, typing.Tuple]"), |
There was a problem hiding this comment.
Again, doesn't Sequence include Tuple already?
| interval: int = 1000, | ||
| progress: Optional[Output] = None, | ||
| progress_default: Any = None, | ||
| running: Optional[List[Tuple[Output, Any, Any]]] = None, |
There was a problem hiding this comment.
took me a moment to parse this :-)
There was a problem hiding this comment.
It's given as [(Output('id', 'prop), "on", "off")]
There was a problem hiding this comment.
I think you can just stop at a higher level Optional[List], and be les concrete, leave room for future modifications.
|
|
||
|
|
||
| # pylint: disable=unused-argument,too-many-locals | ||
| # pylint: disable=unused-argument,too-many-locals,too-many-branches |
| shape_template.format( | ||
| name=name, values=" {\n" + ",\n".join(props) + "\n }" | ||
| ), | ||
| " ", |
There was a problem hiding this comment.
why do we need to indent an empty string at this point?
| # This wrapper adds an argument given to generated Component.__init__ | ||
| # with the actual given parameters by the user as a list of string. | ||
| # This is then checked in the generated init to check if required | ||
| # props were provided. |
There was a problem hiding this comment.
thank you for the explanation
dash/development/component_loader.py
Outdated
| warnings.warn( | ||
| DeprecationWarning( | ||
| "Dynamic components loading has been deprecated and will be removed" | ||
| " in dash 3.0.\n" |
There was a problem hiding this comment.
is there an issue filed to removed dynamic component loading so we don't forget?
| assert ex_err in error | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
| def __init__(self, children=None, optionalArray=Component.UNDEFINED, optionalBool=Component.UNDEFINED, optionalFunc=Component.UNDEFINED, optionalNumber=Component.UNDEFINED, optionalObject=Component.UNDEFINED, optionalString=Component.UNDEFINED, optionalSymbol=Component.UNDEFINED, optionalNode=Component.UNDEFINED, optionalElement=Component.UNDEFINED, optionalMessage=Component.UNDEFINED, optionalEnum=Component.UNDEFINED, optionalUnion=Component.UNDEFINED, optionalArrayOf=Component.UNDEFINED, optionalObjectOf=Component.UNDEFINED, optionalObjectWithExactAndNestedDescription=Component.UNDEFINED, optionalObjectWithShapeAndNestedDescription=Component.UNDEFINED, optionalAny=Component.UNDEFINED, customProp=Component.UNDEFINED, customArrayProp=Component.UNDEFINED, id=Component.UNDEFINED, **kwargs): | ||
| def __init__( | ||
| self, | ||
| children: typing.Optional[typing.Union[str, int, float, ComponentType, typing.Sequence[typing.Union[str, int, float, ComponentType]]]] = None, |
There was a problem hiding this comment.
We can use type-aliases to make our lives easier here.
BasicType: TypeAlias = Union[str, int, float]
ComponentOrBasic: TypeAlias = Union[BasicType, ComponentType]
ChildrenType: TypeAlias = Optional[Union[ComponentOrBasic, Sequence[ComponentOrBasic]]]
children: ChildrenType = None,
Add python
typingto generated components__init__.Contributor Checklist
CHANGELOG.md