Skip to content

Conversation

@waralex
Copy link
Collaborator

@waralex waralex commented Oct 23, 2021

Removed unnecessary validation checks of callbacks parameters in accordance with the issue #137
I also added an analog of FalseList (from https://github.com/plotly/dash/blob/71401b5f9e8ddd835761cd080fa06562eb0523e1/dash/_callback_context.py) for the context.triggered in case when triggered_inputs are empty

@waralex waralex requested a review from alexcjohnson October 23, 2021 12:46
@github-actions github-actions bot added the tests label Oct 23, 2021
@waralex waralex linked an issue Oct 23, 2021 that may be closed by this pull request
if !isempty(changed_props)
triggered = TriggeredParam[(prop_id = id, value = input_values[id]) for id in changed_props]
else
triggered = FakeTriggeredParams()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Python side we did this because for a long time callback_context.triggered always had at least one element in it - in fact, nearly always exactly one - so it was a very common pattern for users to just look at the first list item without first checking that it exists, and when we changed it so in some cases there's nothing in the list we didn't want all that code to error out. Is there a lot of existing Dash.jl user code that will break without this? If not, I'd leave it out, it'll be cleaner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clearly not used now in Julia because it didn't work that way before. I decided to do this for uniformity with python and in order for the example from the issue #137 to work as is. However, if it is not necessary, then I will remove it, because it is confusing logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK good, that's what I thought - let's remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in da85fa9

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
Copy link
Contributor

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

@waralex waralex merged commit 4190e5d into dev Oct 25, 2021
@alexcjohnson alexcjohnson deleted the circular_callbacks branch October 25, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error while using circular callback

2 participants