Refactor PlotSchema #1144

etpinard · 2016-11-15T00:33:34Z

This PR significantly refactor PlotSchema.get() in an effort to reduce the level of (hard-coded) abstractions required to built the schema and to make it play better for custom modules.

In brief,

the circa 2014 _nestedModules and _composedModules attribute abstractions are gone
Plotly.registry now builds Plots.layoutArrayContainer automatically cc @rreusser
add frames and animation attributes to Plotly.PlotSchema.get() output
makes array containers schema tree creation more robust - see b56bf0c
fix bug where rangeslider and rangeselector where listed as available under layout.yaxis
removes some useless exports on Annotations
removes a bunch of circular dependencies cc De-circularize plotly.js require statements #236

- instead require attribute module into trace attribute modules directly.

- that abstraction was mostly obsolete - the only remaining 'composed-module' was Fx, simply merge its attribute into base layout attributes.

- remove _nestedModule and _composedModule handlers - find module attribute from registry (as opposed to the internal Plotly object and other obsolete methods) - add layoutNodes to better handler rangeslider and rangeselector

- and lower the max circular dep limit

- (now that this doesn't cause circular deps anymore)

- instead of simply removing the trailing 's' of the container name

- instead of relying on a hard-coded list

etpinard · 2016-11-15T14:57:57Z

Commit e9f679a is a little hard to read. I'd recommend reviewing plot_api/plot_schema.js as if it were a new file.

rreusser

Approving since I have no specific requirements for changes, but I think it would be nice to very briefly document a few of the parts that touch things fairly close to the surface, in particular the attribute flags and what sort of effect they have.

rreusser · 2016-11-15T14:58:30Z

src/components/annotations/attributes.js

@@ -15,7 +15,7 @@ var extendFlat = require('../../lib/extend').extendFlat;


 module.exports = {
-    _isLinkedToArray: true,
+    _isLinkedToArray: 'annotation',


I've never been able to figure out quite what _isLinkedToArray actually does. It seems to remove the key and trigger some sort of crawl with a key of length - 1 which seems like the sort of thing that would de-pluralize, but this is just a guess.

Perhaps it's a way of defining, for example, button attrs but using a buttons container?

I've never been able to figure out quite what _isLinkedToArray

It's essentially the plot schema version of Plots.layoutArrayContainers. With this PR, both are now united.

Giving a name to the items in the array container is (again) only relevant for plotly.py at the moment. Previously, the item names were simply the container name minus the 's'. This PR make item naming more robust / flexible.

rreusser · 2016-11-15T15:10:29Z

tasks/test_syntax.js

@@ -105,7 +105,7 @@ function assertCircularDeps() {
        // as of v1.17.0 - 2016/09/08
        // see https://github.com/plotly/plotly.js/milestone/9
        // for more details
-        var MAX_ALLOWED_CIRCULAR_DEPS = 34;
+        var MAX_ALLOWED_CIRCULAR_DEPS = 17;


rreusser · 2016-11-15T15:20:13Z

src/plot_api/plot_schema.js

+var IS_SUBPLOT_OBJ = '_isSubplotObj';
+var IS_LINKED_TO_ARRAY = '_isLinkedToArray';
+var DEPRECATED = '_deprecated';
+var UNDERSCORE_ATTRS = [IS_SUBPLOT_OBJ, IS_LINKED_TO_ARRAY, DEPRECATED];


Is it possible to document what these do, perhaps here? A general frustration regarding attribute definitions is that I make up things like role or underscored attributes based on copying from similar examples, but I don't really have any idea what they mean or do. In particular:

_deprecated seems like mainly just an annotation. Do the docs use this in deciding how to display things?

_isLinkedToArray seems to have something to do with pluralization and containers, but I haven't figured out what it means beyond that.

_isSubplotObj: Seems like an annotation, perhaps used elsewhere? I can't find that it actually does anything, at least within plotly.js.

role: Not underscored, but in a similar boat. data_array seems to be important for extracting data out of the json. style affects theming in different environments, if I recall correctly. Do the other options like info_array have any effect? I usually write it because it seems friendly to include, but I don't know if it means anything.

There's probably very little be gained in documenting the internals, but attribute definitions seem like a common thing that people without deep knowledge of the internals probably end up touching pretty regularly, so it might be nice to communicate the meaning somewhere.

_deprecated

These are simply deprecated keys that don't show up on https://plot.ly/javascript/reference/ and friends, but that plotly.py (still) considers valid.

_isLinkedToArray

See #1144 (comment)

_isSubplotObj

Subplot layout containers e.g. layout.xaxis , layout.scene can be input as layout.xaxis2 or layout.scene10, Plotly.validate and plotly.py need to know about that.

role

Has no effect in plotly.js (yet). At the moment, it is only used for plotly.py's strip style method which removes all role: 'style' keys from a given figure.

Ah, my mistake. data_array is a valType, not a role. Which affects streambed parsing, I think.

etpinard added 10 commits November 14, 2016 19:22

schema: rm _nestedAttributes abstraction in trace modules

8214577

- instead require attribute module into trace attribute modules directly.

schema: rm _composedModule abstraction

e4a9de5

- that abstraction was mostly obsolete - the only remaining 'composed-module' was Fx, simply merge its attribute into base layout attributes.

schema: refactor PlotSchame.get

e9f679a

- remove _nestedModule and _composedModule handlers - find module attribute from registry (as opposed to the internal Plotly object and other obsolete methods) - add layoutNodes to better handler rangeslider and rangeselector

rm component module for internal plotly index

8cf071b

- and lower the max circular dep limit

move (back) crawl and friends from coerce.js -> plot_schema.js

7a22ca4

- (now that this doesn't cause circular deps anymore)

schema: make _isLinkedToArray define name of array container items

b56bf0c

- instead of simply removing the trailing 's' of the container name

annotations: do not export useless things

b2b3829

lint component module definitions

b9f2c8e

fill in layoutArrayContainer via Register

1c6b292

- instead of relying on a hard-coded list

schema: add frame and animation attributes

f91ebbf

etpinard added status: reviewable bug something broken labels Nov 15, 2016

etpinard added this to the v1.20.0 milestone Nov 15, 2016

lint (use variable instead of duplicating hard-coded strings)

93fa913

rreusser approved these changes Nov 15, 2016

View reviewed changes

etpinard merged commit 0e0928b into master Nov 15, 2016

etpinard deleted the plot-schema-from-registry branch November 15, 2016 15:57

etpinard mentioned this pull request Nov 22, 2016

Config options coerce / schema #1188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor PlotSchema #1144

Refactor PlotSchema #1144

etpinard commented Nov 15, 2016

etpinard commented Nov 15, 2016

rreusser left a comment

rreusser Nov 15, 2016

etpinard Nov 15, 2016

rreusser Nov 15, 2016

rreusser Nov 15, 2016 •

edited

Loading

etpinard Nov 15, 2016

rreusser Nov 15, 2016

Refactor PlotSchema #1144

Refactor PlotSchema #1144

Conversation

etpinard commented Nov 15, 2016

etpinard commented Nov 15, 2016

rreusser left a comment

Choose a reason for hiding this comment

rreusser Nov 15, 2016

Choose a reason for hiding this comment

etpinard Nov 15, 2016

Choose a reason for hiding this comment

rreusser Nov 15, 2016

Choose a reason for hiding this comment

rreusser Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

etpinard Nov 15, 2016

Choose a reason for hiding this comment

rreusser Nov 15, 2016

Choose a reason for hiding this comment

rreusser Nov 15, 2016 •

edited

Loading