search

plotly / dash

Data Apps & Dashboards for Python. No JavaScript Required.
https://plotly.com/dash
MIT License
21.43k stars 2.07k forks source link

provide types in __init__ signatures #554

Open notatallshaw opened 6 years ago

notatallshaw commented 6 years ago

Because the children argument of a component can take a single element or a list, e.g. html.Div(html.Div()) html.Div([html.Div(), html.Div()])

I can easily make the mistake of not putting the children in a list, e.g.: html.Div(html.Div(), html.Div())

In this case I do not get any kind of error, and instead the 2nd Div is stringified and becomes part of the id for the parent component.

In very large applications this is easy to lose. If type information was provided then IDEs like Pycharm would highlight this issue and tools like MyPy would warn before the code was ever run.

Marc-Andre-Rivet commented 5 years ago

@rmarren1 Would this case be covered by https://github.com/plotly/dash/pull/452?

Marc-Andre-Rivet commented 5 years ago

@notatallshaw Transferring this issue over to the Dash repo. Thanks for raising this issue.

notatallshaw commented 5 years ago

@Marc-Andre-Rivet thanks for the update to the issue. I guess this issue would affect all Dash Components, but the Dash HTML Component is definitely the one where it's most pronounced because of the repeated nesting.

As for #452 I could be wrong but I don't see any talk of PEP 484 Type Annotations in that discussion, but instead a run time check of the Dash Callbacks and the Dash Layout.

mungojam commented 5 years ago

We lost half an hour on this simple bug in our code today. In our case it threw an error that the string format of the id was incorrect. We were confused for ages as we couldn't tell which exact component was firing the error either.

T4rk1n commented 5 years ago

I got static typing support with mypy coming with: https://github.com/plotly/dash/issues/467 & https://github.com/plotly/dash/issues/464

The refactor is working but it's in a heavy work in progress state coupled with #464, I was thinking the docstrings changes were good enough on it's own to refactor it out of the hooks which is an overhaul of the component system.

notatallshaw commented 5 years ago

@T4rk1n Thanks for the work and information!

I assume you mean using Type checking that is specified in a comment above the docstring? As described here and here.

I agree this makes sense for such a widely used library as it is compatible with all Python versions and does not have to worry about current type annotations vs. __future__ type annotations.

I created some example of this by editing some Dash Components using IntelliJ ultimate + Python plugin (my office IDE) and it worked great! IntelliJ highlighted there was problem before I needed to run any code.

T4rk1n commented 5 years ago

@notatallshaw Yes

The PEP 484 suggested syntax for python 2.7 is the most supported by IDE and editors, Pycharm/intelliJ will pick them up right away by adding the type comment to the component __init__ docstring, but vscode and other smart editors needs a plugin for mypy linting and proper generated python filename for the components.

You can also run mypy from the cli on the dash application.