-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single input #1180
Single input #1180
Conversation
This is an interesting idea. A single Note that in the case of @chriddyp thoughts about this? On the one hand I like removing the extra brackets, and I've made the mistake myself many times of forgetting them. On the other hand it gets away from the one-way-to-do-everything philosophy a bit. |
Yeah, I like getting away from square brackets too. I've forgotten them before too. It also makes getting the indentation right difficult: @app.callback(
Output(...),
[
Input(...),
Input(...)
],
[
State(...),
State(...)
]
) vs @app.callback(
Output(...),
Input(...),
Input(...),
State(...),
State(...)
) Re output symmetry - The So, I'd vote for moving away to no lists for I think we can do this in a backwards compatible way, right? If we did this, we'd move all documentation over to this new syntax. As an aside... In general when we consider explicit list vs With One of the original motivations with I don't think that this is a very strong argument though, as our pedagogy will immediately follow up with an example with two inputs. And this still looks pretty intuitive I think: @app.callback(
Output('my-output', 'children'),
Input('my-dropdown-1', 'value'),
Input('my-dropdown-2', 'value'))
def update_output(value1, value2):
return 'You have selected {} and {}'.format(value1, value2) So, I think I'd be in favor of moving away from square brackets all together in a backwards compatible way. |
I suppose as long as we still allow the brackets, and when you DO supply a single output in brackets we still require the return value to be a length-1 list, I think you're right and we can do this in a backward-compatible way.
There is still a use case for this in programmatically-generated callbacks where you don't know how many outputs you'll have, whether it'll be one or more. In that case you'd want your return to always be wrapped in a length-1 list. Though even that use case becomes much less important with pattern-matching callbacks 🎉 |
The latest commits implement the behavior presented by @chriddyp in this comment. This allows to include Side note: I don't know what is going on with Github, as this pull request shows around 90 commits whereas the comparison of the branches shows only 12, which is the real number of changes. |
08ddc9c
to
775bf09
Compare
@mbegel @wilhelmhb apologies for the delay getting back to this PR. I've attempted to rebase off |
@wilhelmhb thanks for generalizing this. One thing I'd like to modify:
I think it's still important to have the items in order: outputs, inputs, state, rather than mixing them up. It would be super confusing to have outputs mixed in with the others, so definitely we need those to be first. Less confusing to have inputs and state mixed up, and I can see the rationale of grouping related items, but the distinction between inputs and state is important enough to the logic of the callback that I think it's worth keeping them separate. FYI the piece that made rebasing tricky (and that I appear to have messed up somewhere) is |
Allow to use named parameters in callback Raise exceptions if outputs, inputs or states are not ordered
Thanks for rebasing, it's much cleaner this way =) My latest commit addresses these two concerns:
Unfortunately, I'm not able to run the lint test on my machine, and I can't access the detailed CI results anymore. |
get some more of them out of the old test_integration
@Marc-Andre-Rivet I think this is ready for a final review. |
dash/_validate.py
Outdated
"then all Inputs, then all States. Found this out of order: | ||
{} | ||
""".format( | ||
repr(extra_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running against
@app.callback(
Output('table', 'data'),
State('table', 'data_previous'),
Input('btn', 'n_clicks'),
State('table', 'data')
)
Got the error message:
dash.exceptions.IncorrectTypeException: In a callback definition, you must provide all Outputs first,
"then all Inputs, then all States. Found this out of order:
[<Input `btn.n_clicks`>, <State `table.data`>]
As the error causing props are from the previous State('table', 'data_previous')
and Input('btn', 'n_clicks')
, the message could be tweaked to be more helpful / point to the problematic parameters / relationship more precisely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. In d158ef8 I changed it so this particular situation would say:
dash.exceptions.IncorrectTypeException: In a callback definition, you must provide all Outputs first,
then all Inputs, then all States. After this item:
<State `table.data_previous`>
we found this item next:
<Input `btn.n_clicks`>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃
Merging - to be included in Dash 1.15, should come out in the next couple of weeks. Thanks for your contributions @mbegel and @wilhelmhb ! |
Single
Input
in callbacks don't need to be in a list.