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

Allow Dash children lists to be tuples also. #358

Closed
rmarren1 opened this issue Aug 29, 2018 · 7 comments
Closed

Allow Dash children lists to be tuples also. #358

rmarren1 opened this issue Aug 29, 2018 · 7 comments
Assignees

Comments

@rmarren1
Copy link
Contributor

Right now we only accept objects which are instances of collections.MutableSequence as lists in Dash.

Is there a reason we are this specific? A tuple will be serialized just fine.

@rmarren1 rmarren1 changed the title (Idea) Allow collections.Sequence as lists in Dash layout Allow collections.Sequence as lists in Dash layout Aug 29, 2018
@T4rk1n
Copy link
Contributor

T4rk1n commented Aug 29, 2018

If it serialize, then I see no reason not to accept them.

@chriddyp
Copy link
Member

we only accept objects which are instances of collections.MutableSequence as lists in Dash.

By 'only accept', do you mean in the plotly.utils.PlotlyJSONEncoder or in the base_component.Component?

@rmarren1
Copy link
Contributor Author

rmarren1 commented Sep 1, 2018

PlotlyJSONEncoder looks like it allows tuples.

It looks like the problem is just that the base component traversal uses isinstance(x, collections.MutableSequence) to check if an object is a list here and here and here and here.

I think if we just change these lines to collections.Sequence we should get support for tuples.

@chriddyp
Copy link
Member

chriddyp commented Sep 4, 2018

image

Yeah, I don't think that we're using any of those extra MutableSequence methods (append, reverse, extend, pop, remove, __iadd__), so I think we're good. (https://docs.python.org/2/library/collections.html#collections-abstract-base-classes)

@rmarren1
Copy link
Contributor Author

rmarren1 commented Sep 4, 2018

I'll make a PR and see how it works out

@rmarren1 rmarren1 self-assigned this Sep 4, 2018
@rmarren1 rmarren1 changed the title Allow collections.Sequence as lists in Dash layout Allow Dash children lists to be tuples also. Oct 18, 2018
@rmarren1
Copy link
Contributor Author

It turns out making Dash children a tuple instead of a list (e.g. html.Div(children=(html.P("Hello"), html.P("World"))) already works, but the Component.traverse() method does not work correctly in these instances so some things break like layout validation, JSON serializable error checking, etc

@rmarren1
Copy link
Contributor Author

strings are instances of collections.Sequence, so to get the desired behavior I just added tuple to the list of accepted types, e.g. isinstance(x, (tuple, collections.MutableMapping)) instead of isinstance(x, collections.MutableMapping)

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

No branches or pull requests

3 participants