Skip to content

nestedProperty results in modal Plotly.restyle #1580

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

Closed
rreusser opened this issue Apr 11, 2017 · 5 comments
Closed

nestedProperty results in modal Plotly.restyle #1580

rreusser opened this issue Apr 11, 2017 · 5 comments

Comments

@rreusser
Copy link
Contributor

rreusser commented Apr 11, 2017

See: http://codepen.io/rsreusser/pen/NjKXqJ?editors=0010

It seems to me that Plotly.restyle is modal in an undesirable way. That is, the same command succeeds or fails depending on the value of current data. Right now value of the filter transform is the only property which I'm aware exhibits this since either a number or an array is valid, but there may be others.

Consider:

  1. value is a number
  2. restyle 'value[0]' ⟶ fails because you can't set property [0] of a number
  3. restyle 'value': [0, 0] ⟶ succeeds
  4. restyle 'value[0]' ⟶ succeeds

So the same command both fails and succeeds. You're welcome to close this, but it seems undesirable and uncommon, so I'd love to answer it definitively before it becomes a common usage that can't be undone. (Which is to say, carpet adds another usage of this and I'm currently struggling with the fallout.)

@etpinard
Copy link
Contributor

Interesting. Thanks for bringing this up @rreusser 🍻

What does relayout do with e.g axis range and domain?

@rreusser
Copy link
Contributor Author

rreusser commented Apr 11, 2017

That case seems to work as expected. It seems like a bare number n gets interpreted as the array [n, n + 1]. In that case though, there's no confusion since the range itself can't be a bare number. The confusion here is due to the fact that {operation: '<', value: 5} expects a scalar value while {operation: '[]', value: [4, 5]} expects an array range. Both are fine independently, but it's the passing between them via restyle that's problematic.

@rreusser
Copy link
Contributor Author

rreusser commented Apr 11, 2017

One proposed fix: Sanitization is generally (always? by definition?) done at the supplydefaults level. So if nestedProperty complied and expanded values into an array when requested to set a particular index, perhaps that would resolve this, but then maybe it would cause problems elsewhere. cc @alexcjohnson for ever-insightful API input.

That is, say value is 5. You try to set 'value[1]': 6. First it could wrap 5, then assign your value. You'd get the result [5, 6]. I feel like that would be more or less unsurprising to me.

Either that or modal number-array attributes should be disallowed and it would require {operation: '<', value: [5]}

@alexcjohnson
Copy link
Collaborator

My concern about this is that in general (including most arrayOk cases), 5 and [5] are not equivalent. When used for alongside a data array, 5 applies to all points whereas [5] applies to the first point and the rest receive some sort of default value. Even in this case, you wouldn't just change from 5 to [5, 6], would you? You'd change operation as well, to [] or something, and you also can't use 5 with [] so they necessarily need to happen together.

You could say then that we should automatically wrap or unwrap the value... but I don't think that works either: depending on which operation you changed to, the "natural" place to put the 5 depends on which operation you're coming from and going to... eg '<' to '[]' the 5 would naturally belong in index 1, but going to '][' it would belong in 0. And if you're unwrapping, what would you do with the extra value? Which one is extra?

So it seems to me safer to leave it as is, that the user must provide the whole object if she wishes to change its type between bare value, array, and hash.

@rreusser
Copy link
Contributor Author

Agreed. My takeaways:

  1. it's probably best for the client to resolve the intent. If you want to set two values simultaneously, then the widget should probably assign the array value as one instead of two independent inputs.
  2. small API surface area is good, but polymorphic array/scalar fields are probably best avoided when it's not too cumbersome.

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