Custom response support from callbacks redux #401
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging @Plotly/dash as suggested in the CONTRIBUTING.md pull request (but perhaps that team doesn't exist yet?) |
|
thanks! cc @plotly/dash |
T4rk1n
requested changes
Sep 24, 2018
dash/response.py
Outdated
| other properties of the response like headers or cookies. | ||
| """ | ||
| def __init__(self, response, **kwargs): | ||
| self.dash_response = response |
There was a problem hiding this comment.
Let's call that output_value instead since it is the return value of a callback and not a response.
dash/response.py
Outdated
| def jsonify_response(self, output, validator): | ||
| """ | ||
| convert response to valid Dash json-encoded | ||
| :param output: output element for the callback |
There was a problem hiding this comment.
Docstring need a space before the :param: to be valid, also capitalize.
dash/response.py
Outdated
| '', # filled in by set_data later | ||
| mimetype='application/json', **kwargs) | ||
|
|
||
| def jsonify_response(self, output, validator): |
There was a problem hiding this comment.
Dash._validate_callback_output can be made static, we should move it in DashResponse and remove the output_value to be the instance value.
dash/response.py
Outdated
| other properties of the response like headers or cookies. | ||
| """ | ||
| def __init__(self, response, **kwargs): | ||
| self.dash_response = response |
There was a problem hiding this comment.
I'd like the output value to be property that does the jsonify_response automatically when set so we don't have to call that from dash. A benefit would be that the stacktrace would be closer to where the output_value was set and easier to debug for the user.
dash/response.py
Outdated
| id=output.component_id)) | ||
|
|
||
| self.set_data(json_value) | ||
| return self |
There was a problem hiding this comment.
⚡ There is no need to make this method tail, there is only one call and no other methods will be called after.
|
Sorry, look like I missed the output param of |
|
Thanks for the review! |
T4rk1n
approved these changes
Sep 25, 2018
dash/response.py
Outdated
| Return a `DashResponse` object from a Dash callback in order to set | ||
| other properties of the response like headers or cookies. | ||
| """ | ||
| def __init__(self, response, **kwargs): |
There was a problem hiding this comment.
Should be output_value instead of response for uniformity between the arg and the instance attribute.
|
Sorry for the delay, this looks ready. Can you rebase/merge master to fix the conflicts ? I'll merge/release as soon as it's done. |
ad03afd to
67f3611
Compare
|
Rebased. Somehow, the test no longer passed on CircleCI although it worked fine for me locally. It seems that the |
|
In the rebase, I had to replicate the changes made to the code that I moved into a different file. I don't know if Github has any way to see what happened there, but here's how I checked for myself on the command line: git blame -M -C -w -L '/def _validate_callback_output/,/TODO/' master dash/dash.py > old
git blame -M -C -w -L '/def _validate_callback_output/,' callback-response-support dash/response.py > new
paste old new | expand -t 150 | lessThe lines don't match exactly because I removed a redundant argument to git blame outputs side-by-side
|
|
The input1 clear test failure is due to a bug in dcc. We locked the version on master, you can rebase again sorry. That bug is almost impossible to reproduce by hand, it's a question of timing with the callbacks I think. @rmarren1 Can you have a look at the diff, this PR moved the callback validation method. |
This way there is only one place with the json logic and the related error handling.
Rename dash_response to output_value, improve documentation, don't make the jsonify call be in tail position.
67f3611 to
b119577
Compare
|
Still getting |
For whatever reason, input1.clear() didn't clear the input field on CircleCI although it was fine when running tests locally.
|
Sorry, we locked the versions for the call count failures not the initial-value that appear in your new test. plotly/dash-core-components#384 adds a test for that bug, if the ActionChain clear works then it's good for the new test. |
|
@T4rk1n is anything else needed or can we merge this? |
|
We could see if the test change in that last commit is still necessary. Perhaps #384 makes it unnecessary? I can't test right now. |
We need to validate with @rmarren1 the move of the callback validation method, not sure if it conflicts with the validation PR.
The test is good, selenium has a bug with the input not firing a onChange when cleared. The ActionChain simulate a manual clear by selecting all the text and deleting it so it fires a onChange to the input. |
|
I was about to update this PR to the latest master - and we could certainly do that - but I wonder if it would make more sense to extend the pattern we started in #608 with Either way the user needs to know a new piece of syntax - either the Thoughts @jkseppan @plotly/dash-core ? |
|
@alexcjohnson That does seem like a good design, especially given the symmetry with |
|
Superseded by #623 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on @oryba's PR #183, this adds an integration test and moves the json encoding and error handling into one place, as requested in the comments to that PR.