8000 Common interface to interpret and execute API methods by rreusser · Pull Request #1016 · plotly/plotly.js · GitHub
[go: up one dir, main page]
Skip to content

Common interface to interpret and execute API methods #1016

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.

Sign up for GitHub

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

Merged
merged 29 commits into from
Oct 25, 2016
Merged

Common interface to interpret and execute API methods #1016

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f2d268d
Start implementing command execution wrapper
rreusser Oct 4, 2016
65f618d
Write failing tests for binding computation
rreusser Oct 6, 2016
7ef0404
Add a lot of tests for binding computation
rreusser Oct 10, 2016
edb6773
Implement animate and update binding computation
rreusser Oct 10, 2016
f50fcaa
Switch return format of bindings to structured output
rreusser Oct 11, 2016
60dd634
Merge remote-tracking branch 'origin/master' into lib-commands
rreusser Oct 11, 2016
4bc8387
Command to decide when bindings are simple
rreusser Oct 17, 2016
93b8253
First working version of bindings
rreusser Oct 18, 2016
43dc3d4
Change the method name
rreusser Oct 18, 2016
65d24f3
Fix updatemenus bindings
rreusser Oct 18, 2016
d2696e7
Hook up animations to sliders via bindings
rreusser Oct 18, 2016
7db7f03
Clean up slider positioning code
rreusser Oct 20, 2016
57fcf96
Add mock for bindings
rreusser Oct 20, 2016
7db5f1f
emove custom plotmodified event
rreusser Oct 20, 2016
26ad1ff
Add binding baseline image
rreusser Oct 20, 2016
986b4dc
Fix irritating self-interaction when dragging slider
rreusser Oct 24, 2016
20ac69f
Fix lint issue
rreusser Oct 24, 2016
f993ed7
Change failure modes for command API
rreusser Oct 24, 2016
99d7c8e
Ugh linter again
rreusser Oct 24, 2016
f8c0094
Improve robustness o 8000 f command bindings
rreusser Oct 24, 2016
6abec15
Test more behavior of bindings
rreusser Oct 24, 2016
5df94a7
Fix linter issue
rreusser Oct 24, 2016
d35ee35
Remove binding test file
rreusser Oct 24, 2016
a554cad
Add equivalent command API test for udpatemenus
rreusser Oct 24, 2016
e5a80ee
DRY up binding change check
rreusser Oct 24, 2016
45717b1
Add note about test failure
rreusser Oct 24, 2016
52de9e4
createCommandObserver --> manageCommandObserver
rreusser Oct 25, 2016
0c40b02
Remove hard-coded updatemenus active default
rreusser Oct 25, 2016
df2d5bb
Revert updatemenus initialization and fix sliders initialization
rreusser Oct 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Write failing tests for binding computation
  • Loading branch information
@rreusser
rreusser committed Oct 6, 2016
commit 65f618dc1960fa23c9483e97d3b3d5e204bedc93
60 changes: 40 additions & 20 deletions src/plots/command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@
'use strict';

var Plotly = require('../plotly');
var Lib = require('../lib');
var helpers = require('../plot_api/helpers');

exports.executeAPICommand = function(gd, method, args) {
var apiMethod = Plotly[method];

var allArgs = [gd];
for (var i = 0; i < args.length; i++) {
for(var i = 0; i < args.length; i++) {
allArgs.push(args[i]);
}

if (!apiMethod) {
if(!apiMethod) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That will never happen, no? Isn't apiMethod guaranteed to be defined after supplyDefaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I have no strong feelings either way; it's just a general habit analogous to callback && callback() in order to ensure I never unexpectedly invoke undefined(). Thought maybe safety for depending on how it gets used in the future, but I'm fine just removing. I'm not great at proper promise error handling. Few are.

8000
Copy link
Contributor

Choose a reason for hiding this comment

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

Our supply-default machinery is pretty robust. If apiMethod somehow becomes undefined at this stage in the future one or many test cases will fail.

So, I'd vote for removing these useless checks and instead maybe add a comment about the function declaration saying this function assumes a full round of supply-defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed check 👍

return Promise.reject();
}

Expand All @@ -30,30 +31,49 @@ exports.executeAPICommand = function(gd, method, args) {
exports.computeAPICommandBindings = function(gd, method, args) {

switch(method) {
case 'restyle':
var traces
case 'restyle':
var traces;

// Logic copied from Plotly.restyle:
var astr = args[0];
var val = args[1];
var traces = args[2];
var aobj = {};
if(typeof astr === 'string') aobj[astr] = val;
else if(Lib.isPlainObject(astr)) {
var astr = args[0];
var val = args[1];
var traces = args[2];
var aobj = {};
if(typeof astr === 'string') aobj[astr] = val;
else if(Lib.isPlainObject(astr)) {
// the 3-arg form
aobj = astr;
if(traces === undefined) traces = val;
} else {
aobj = astr;
if(traces === undefined) traces = val;
} else {
// This is the failure case, but it's not a concern of this method to fail
// on bad input. This will just return no bindings:
return [];
}
return [];
}

console.log('aobj:', aobj);
console.log('aobj:', aobj);

return ['data[0].marker.size'];
break;
default:
throw new Error('Unimplemented');
return ['data[0].marker.size'];
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

again here, method should always be defined at the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess really it should throw so that when we add api methods and someone forgets to whitelist them here, they'll get a hard error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now throws error.

// The unknown case. We'll elect to fail-non-fatal since this is a correct
// answer and since this is not a validation method.
return [];
}
};

function crawl(attrs, callback, path) {
if(path === undefined) {
path = '';
}

Object.keys(attrs).forEach(function(attrName) {
var attr = attrs[attrName];

if(exports.UNDERSCORE_ATTRS.indexOf(attrName) !== -1) return;

callback(attr, attrName, attrs, level);

if(isValObject(attr)) return;
if(isPlainObject(attr)) crawl(attr, callback, level + 1);
});
}
107 changes: 101 additions & 6 deletions test/jasmine/tests/command_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('Plots.executeAPICommand', function() {
beforeEach(function() {
spyOn(PlotlyInternal, 'restyle').and.callFake(function() {
return Promise.resolve('resolution');
})
});
});

it('calls the API method and resolves', function(done) {
Expand All @@ -43,7 +43,7 @@ describe('Plots.executeAPICommand', function() {
beforeEach(function() {
spyOn(PlotlyInternal, 'restyle').and.callFake(function() {
return Promise.reject('rejection');
})
});
});

it('calls the API method and rejects', function(done) {
Expand All @@ -68,7 +68,10 @@ describe('Plots.computeAPICommandBindings', function() {
beforeEach(function() {
gd = createGraphDiv();

Plotly.plot(gd, [{x: [1, 2, 3], y: [4, 5, 6]}]);
Plotly.plot(gd, [
{x: [1, 2, 3], y: [1, 2, 3]},
{x: [1, 2, 3], y: [4, 5, 6]},
]);
});

afterEach(function() {
Expand All @@ -77,10 +80,102 @@ describe('Plots.computeAPICommandBindings', function() {

describe('restyle', function() {
describe('astr + val notation', function() {
it('computes the binding', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', 7]);
describe('with a single attribute', function() {
it('with a scalar value', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', 7]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size']);
});

it('with an array value and no trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7]]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size']);
});

it('with trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', 7, [0]]);
expect(result).toEqual(['data[0].marker.size']);
});

it('with a different trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', 7, [1]]);
expect(result).toEqual(['data[1].marker.size']);
});

it('with an array value', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7], [0]]);
expect(result).toEqual(['data[1].marker.size']);
});

it('with two array values and two traces specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7, 5], [0, 1]]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size']);
});

it('with traces specified in reverse order', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7, 5], [1, 0]]);
expect(result).toEqual(['data[1].marker.size', 'data[0].marker.size']);
});

it('with two values and a single trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7, 5], [0]]);
expect(result).toEqual(['data[0].marker.size']);
});

it('with two values and a different trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', ['marker.size', [7, 5], [1]]);
expect(result).toEqual(['data[1].marker.size']);
});
});
});

describe('aobj notation', function() {
describe('with a single attribute', function() {
it('with a scalar value', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': 7}]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size']);
});

it('with trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': 7}, [0]]);
expect(result).toEqual(['data[0].marker.size']);
});

it('with a different trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': 7}, [1]]);
expect(result).toEqual(['data[1].marker.size']);
});

it('with an array value', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': [7]}, [0]]);
expect(result).toEqual(['data[1].marker.size']);
});

it('with two array values and two traces specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': [7, 5]}, [0, 1]]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size']);
});

it('with traces specified in reverse order', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': [7, 5]}, [1, 0]]);
expect(result).toEqual(['data[1].marker.size', 'data[0].marker.size']);
});

it('with two values and a single trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': [7, 5]}, [0]]);
expect(result).toEqual(['data[0].marker.size']);
});

it('with two values and a different trace specified', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': [7, 5]}, [1]]);
expect(result).toEqual(['data[1].marker.size']);
});
});

expect(result).toEqual(['data[0].marker.size']);
describe('with multiple attributes', function() {
it('with a scalar value', function() {
var result = Plots.computeAPICommandBindings(gd, 'restyle', [{'marker.size': 7, 'text.color': 'blue'}]);
expect(result).toEqual(['data[0].marker.size', 'data[1].marker.size', 'data[0].text.color', 'data[1].text.color']);
});
});
});
});
Expand Down
0