Skip to content
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

Flexible expanded callback #304

Merged

Conversation

sdementen
Copy link
Contributor

allow a more flexible use of expanded_callback:

  • no obligation to adapt the signature of a callback (by adding a **kwargs) when going from app.callback to app.expanded_callback
  • allow to specify the name of the specific extra parameters needed by the function (no need for **kwargs) to improve readability like in
# current version
@app.expanded_callback(...)
def my_callback(..., **kwargs): ...

# new flexible version : no need to add **kwargs if not needed
@app.expanded_callback(...)
def my_callback(...): ...

# new flexible version : just get two of the new parameters (user and callback_context
@app.expanded_callback(...)
def my_callback(..., user, callback_context): ...

# new flexible version : get two of the new parameters and the rest in **kwargs
@app.expanded_callback(...)
def my_callback(..., user, callback_context, **kwargs): ...

- no obligation to adapt the signature of a callback when going from app.callback to app.expanded_callback
- allow to specify the name of the specific extra parameters needed by the function (no need for **kwargs) to improve readability
@sdementen
Copy link
Contributor Author

@GibbsConsulting I wonder even if, with the introspection of the callback signature, the distinction between expanded_callback and callback is necessary anymore...

Maybe a simple option at the DjangoDash(..., use_dash_dispatch=True|False) with a default to False would be cleaner.

@sdementen
Copy link
Contributor Author

fix #306

@GibbsConsulting
Copy link
Owner

@sdementen yes, simplifying the callback/extended_callback approach is a great idea. Thanks for these contributions, most appreciated.

@sdementen
Copy link
Contributor Author

Would using always the django dispatching be an option? Or do you think it is worth keeping the dash direct dispatch?
If the first option, then we could make all callback automatically expanded_callbcks with the trick of specifying in the callback function the name of the extra pdp arguments that pdp would inject automatically (the logic in this PR)

- always use dpd dispatch (remove the _expanded_callbacks variable and expanded_callbacks arguments in __init__, remove use_dash_dispatch())
- refactor the callback inspection logic to get_expanded_arguments and test it explicitly
- refactur callback_set to use [] instead of {} as default
- expanded_callback = callback
- fix bug in handling callback.expanded
- update doc
@sdementen
Copy link
Contributor Author

I have integrated the expanded_callback functionality in the standard callback (now both functions are the same). It should be backward compatible with django dash app that are only using the normal arguments

@GibbsConsulting
Copy link
Owner

Other than any backwards compatibility issue, I cant see a reason for keeping the dash direct dispatch option. However, other than a possible extra overhead, which itself could not be large, I am unable to construct an immediate example of something that wouldn't work as a result of this change.

django_plotly_dash/dash_wrapper.py Outdated Show resolved Hide resolved
@GibbsConsulting GibbsConsulting merged commit b2d160f into GibbsConsulting:master Jan 24, 2021
@sdementen sdementen deleted the flexible-expanded-callback branch January 24, 2021 09:17
@zvi-quantivly
Copy link

zvi-quantivly commented Mar 3, 2024

I am unable to construct an immediate example of something that wouldn't work as a result of this change.

Just for completeness’ sake, mentioning here #484, which, IIUC, is caused by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants