Skip to content

Templates #2761

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

Merged
merged 24 commits into from
Jul 3, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b8950c5
fix devtools timeit sorting
alexcjohnson Jun 19, 2018
355fb09
stop pushing tickmode back to axis in for a particular edge case
alexcjohnson Jun 18, 2018
8ddcfd6
minor simplification in pie defaults
alexcjohnson Jun 21, 2018
b5ccfbe
combine annotation defaults files & shape defaults files
alexcjohnson Jun 21, 2018
86e09bb
add visible attribute to lots of container array items:
alexcjohnson Jun 20, 2018
3dee25c
use handleArrayContainerDefaults for various array containers:
alexcjohnson Jun 21, 2018
5cca8d3
templates - big commit implementing most of it, coerce-level integration
bpostlethwaite Apr 12, 2018
b3da4e7
:hocho: itemIsNotPlainObject from handleArrayContainerDefaults
alexcjohnson Jun 26, 2018
8525953
add template attributes to array containers
alexcjohnson Jun 26, 2018
e306d1c
template-safe axis default color inheritance logic
alexcjohnson Jun 27, 2018
8e2a321
template-safe GUI editing of array objects
alexcjohnson Jun 27, 2018
4346611
template mock
alexcjohnson Jun 27, 2018
fb489aa
tickformatstops.visible -> enabled
alexcjohnson Jun 28, 2018
bc21cc8
:hocho: done TODO
alexcjohnson Jun 28, 2018
8490804
fix test failures - and simplify handleArrayContainerDefaults even mo…
alexcjohnson Jun 28, 2018
c2bcfe3
fix makeTemplate and test it & template interactions
alexcjohnson Jul 1, 2018
890a324
fix Plotly.validate with attributes that end in numbers
alexcjohnson Jul 2, 2018
aed44dc
Plotly.validateTemplate
alexcjohnson Jul 3, 2018
6df61e0
loosen template default item interaction tests to pass on CI
alexcjohnson Jul 3, 2018
ef4c3cc
TODO -> note re: axis.type _noTemplate
alexcjohnson Jul 3, 2018
b1c6f0a
_noTemplating for angularaxis.type
alexcjohnson Jul 3, 2018
818cac9
:cow2: test name typo
alexcjohnson Jul 3, 2018
15931cf
recurse into (template)layout looking for unused containers
alexcjohnson Jul 3, 2018
8598bc9
:hocho: obsolete code
alexcjohnson Jul 3, 2018
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
5 changes: 4 additions & 1 deletion src/plot_api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,7 @@ exports.setPlotConfig = main.setPlotConfig;
exports.toImage = require('./to_image');
exports.validate = require('./validate');
exports.downloadImage = require('../snapshot/download');
exports.makeTemplate = require('./make_template');

var templateApi = require('./template_api');
exports.makeTemplate = templateApi.makeTemplate;
exports.validateTemplate = templateApi.validateTemplate;
2 changes: 2 additions & 0 deletions src/plot_api/plot_template.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ exports.arrayTemplater = function(container, name, inclusionAttr) {
// it's explicitly marked visible - in which case it gets NO template,
// not even the default.
out[inclusionAttr] = itemIn[inclusionAttr] || false;
// special falsy value we can look for in validateTemplate
out._template = false;
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 case is just special because we were told to look for a template by name and it wasn't found.

Otherwise _template can be null (no template was found but that's fine) or undefined (we don't need to propagate the template into this container).

return out;
}

Expand Down
186 changes: 185 additions & 1 deletion src/plot_api/make_template.js → src/plot_api/template_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var dfltConfig = require('./plot_config');
* @returns {object} template: the extracted template - can then be used as
* `layout.template` in another figure.
*/
module.exports = function makeTemplate(figure) {
exports.makeTemplate = function(figure) {
figure = Lib.extendDeep({_context: dfltConfig}, figure);
Plots.supplyDefaults(figure);
var data = figure.data || [];
Expand Down Expand Up @@ -269,3 +269,187 @@ function getNextPath(parent, key, path) {

return nextPath;
}

/**
* validateTemplate: Test for consistency between the given figure and
* a template, either already included in the figure or given separately.
* Note that not every issue we identify here is necessarily a problem,
* it depends on what you're using the template for.
*
* @param {object|DOM element} figure: the plot, with {data, layout} members,
* to test the template against
* @param {Optional(object)} template: the template, with its own {data, layout},
* to test. If omitted, we will look for a template already attached as the
* plot's `layout.template` attribute.
*
* @returns {array} array of error objects each containing:
Copy link
Contributor

@etpinard etpinard Jul 3, 2018

Choose a reason for hiding this comment

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

THanks for making this have a similar API as Plotly.validate !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One difference is I didn't make the object fields consistent - they all have code but each error, in fact even more specific than each code, includes whatever extra data it deems relevant. Given the nature of the issues (some apply to a specific trace, some to a path, some to a trace type but not a specific trace, etc... as opposed to Plotly.validate where you can basically always trace the issue back to a specific path) it seemed a bit to constraining to give them all the same fields.

* - {string} code
* error code ('missing', 'unused', 'reused', 'noLayout', 'noData')
* - {string} msg
* a full readable description of the issue.
*/
exports.validateTemplate = function(figureIn, template) {
var figure = Lib.extendDeep({}, {
_context: dfltConfig,
data: figureIn.data,
layout: figureIn.layout
});
var layout = figure.layout || {};
if(!isPlainObject(template)) template = layout.template || {};
var layoutTemplate = template.layout;
var dataTemplate = template.data;
var errorList = [];

figure.layout = layout;
figure.layout.template = template;
Plots.supplyDefaults(figure);

var fullLayout = figure._fullLayout;
var fullData = figure._fullData;

if(!isPlainObject(layoutTemplate)) {
errorList.push({code: 'layout'});
}
else {
// TODO: any need to look deeper than the first level of layout?
// I don't think so, that gets all the subplot types which should be
// sufficient.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about xaxis.rangeslider.yaxis2 from #2364?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point. You're right, though it's a lot of complication (for example for xaxis.rangeslider.yaxis we'd have to look for xaxis<N>.rangeslider.yaxis<M> for any combination of <N> and <M> and only generate an error if we don't find any of them. And the only time this would alert you that the template isn't appropriate is if the template has a xaxis<N>.rangeslider.yaxis<M> but does NOT have yaxis<M> - ie your template had opinions about how that y axis should work inside the rangeslider, but left the y axis itself untouched. This is of course possible, just seems very unlikely.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is of course possible, just seems very unlikely.

Right. I don't this needs special handling. A comment about this would suffice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually wasn't all that hard - recursed into layout in 15931cf

for(var key in layoutTemplate) {
if(key.indexOf('defaults') === -1 && isPlainObject(layoutTemplate[key]) &&
!hasMatchingKey(fullLayout, key)
) {
errorList.push({code: 'unused', path: 'layout.' + key});
}
}
}

if(!isPlainObject(dataTemplate)) {
errorList.push({code: 'data'});
}
else {
var typeCount = {};
var traceType;
for(var i = 0; i < fullData.length; i++) {
var fullTrace = fullData[i];
traceType = fullTrace.type;
typeCount[traceType] = (typeCount[traceType] || 0) + 1;
if(!fullTrace._fullInput._template) {
// this takes care of the case of traceType in the data but not
// the template
errorList.push({
code: 'missing',
index: fullTrace._fullInput.index,
traceType: traceType
});
}
}
for(traceType in dataTemplate) {
var templateCount = dataTemplate[traceType].length;
var dataCount = typeCount[traceType] || 0;
if(templateCount > dataCount) {
errorList.push({
code: 'unused',
traceType: traceType,
templateCount: templateCount,
dataCount: dataCount
});
}
else if(dataCount > templateCount) {
errorList.push({
code: 'reused',
traceType: traceType,
templateCount: templateCount,
dataCount: dataCount
});
}
}
}

// _template: false is when someone tried to modify an array item
// but there was no template with matching name
function crawlForMissingTemplates(obj, path) {
for(var key in obj) {
if(key.charAt(0) === '_') continue;
var val = obj[key];
var nextPath = getNextPath(obj, key, path);
if(isPlainObject(val)) {
if(Array.isArray(obj) && val._template === false && val.templateitemname) {
errorList.push({
code: 'missing',
path: nextPath,
templateitemname: val.templateitemname
});
}
crawlForMissingTemplates(val, nextPath);
}
else if(Array.isArray(val) && hasPlainObject(val)) {
crawlForMissingTemplates(val, nextPath);
}
}
}
crawlForMissingTemplates({data: fullData, layout: fullLayout}, '');

if(errorList.length) return errorList.map(format);
};

function hasPlainObject(arr) {
for(var i = 0; i < arr.length; i++) {
if(isPlainObject(arr[i])) return true;
}
}

function hasMatchingKey(obj, key) {
if(key in obj) return true;
if(getBaseKey(key) !== key) return false;
for(var key2 in obj) {
if(getBaseKey(key2) === key) return true;
}
}

function format(opts) {
var msg;
switch(opts.code) {
case 'data':
msg = 'The template has no key data.';
break;
case 'layout':
msg = 'The template has no key layout.';
break;
case 'missing':
if(opts.path) {
msg = 'There are no templates for item ' + opts.path +
' with name ' + opts.templateitemname;
}
else {
msg = 'There are no templates for trace ' + opts.index +
', of type ' + opts.traceType + '.';
}
break;
case 'unused':
if(opts.path) {
msg = 'The template item at ' + opts.path +
' was not used in constructing the plot.';
}
else if(opts.dataCount) {
msg = 'Some of the templates of type ' + opts.traceType +
' were not used. The template has ' + opts.templateCount +
' traces, the data only has ' + opts.dataCount +
' of this type.';
}
else {
msg = 'The template has ' + opts.templateCount +
' traces of type ' + opts.traceType +
' but there are none in the data.';
}
break;
case 'reused':
msg = 'Some of the templates of type ' + opts.traceType +
' were used more than once. The template has ' +
opts.templateCount + ' traces, the data has ' +
opts.dataCount + ' of this type.';
break;
}
opts.msg = msg;

return opts;
}
103 changes: 103 additions & 0 deletions test/jasmine/tests/template_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,3 +235,106 @@ describe('template interactions', function() {
.then(done);
});
});

describe('validateTemplate', function() {

function checkValidate(mock, expected, countToCheck) {
var template = mock.layout.template;
var mockNoTemplate = Lib.extendDeep({}, mock);
delete mockNoTemplate.layout.template;

var out1 = Plotly.validateTemplate(mock);
var out2 = Plotly.validateTemplate(mockNoTemplate, template);
expect(out2).toEqual(out1);
if(expected) {
expect(countToCheck ? out1.slice(0, countToCheck) : out1)
.toEqual(expected);
}
else {
expect(out1).toBeUndefined();
}
}

var cleanMock = Lib.extendDeep({}, templateMock);
cleanMock.layout.annotations.pop();
cleanMock.data.pop();
cleanMock.data.splice(1, 1);
cleanMock.layout.template.data.bar.pop();

it('returns undefined when the template matches precisely', function() {
checkValidate(cleanMock);
});

it('catches all classes of regular issue', function() {
var messyMock = Lib.extendDeep({}, templateMock);
messyMock.data.push({type: 'box', x0: 1, y: [1, 2, 3]});
messyMock.layout.template.layout.geo = {projection: {type: 'orthographic'}};
messyMock.layout.template.layout.xaxis3 = {nticks: 50};
messyMock.layout.template.data.violin = [{fillcolor: '#000'}];

checkValidate(messyMock, [{
code: 'unused',
path: 'layout.geo',
msg: 'The template item at layout.geo was not used in constructing the plot.'
}, {
code: 'unused',
path: 'layout.xaxis3',
msg: 'The template item at layout.xaxis3 was not used in constructing the plot.'
}, {
code: 'missing',
index: 5,
traceType: 'box',
msg: 'There are no templates for trace 5, of type box.'
}, {
code: 'reused',
traceType: 'scatter',
templateCount: 2,
dataCount: 4,
msg: 'Some of the templates of type scatter were used more than once.' +
' The template has 2 traces, the data has 4 of this type.'
}, {
code: 'unused',
traceType: 'bar',
templateCount: 2,
dataCount: 1,
msg: 'Some of the templates of type bar were not used.' +
' The template has 2 traces, the data only has 1 of this type.'
}, {
code: 'unused',
traceType: 'violin',
templateCount: 1,
dataCount: 0,
msg: 'The template has 1 traces of type violin' +
' but there are none in the data.'
}, {
code: 'missing',
path: 'layout.annotations[4]',
templateitemname: 'nope',
msg: 'There are no templates for item layout.annotations[4] with name nope'
}]);
});

it('catches missing template.data', function() {
var noDataMock = Lib.extendDeep({}, cleanMock);
delete noDataMock.layout.template.data;

checkValidate(noDataMock, [{
code: 'data',
msg: 'The template has no key data.'
}],
// check only the first error - we don't care about the specifics
// uncovered after we already know there's no template.data
1);
});

it('catches missing template.data', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

🐄 catches missing template.layout.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch! 818cac9

var noLayoutMock = Lib.extendDeep({}, cleanMock);
delete noLayoutMock.layout.template.layout;

checkValidate(noLayoutMock, [{
code: 'layout',
msg: 'The template has no key layout.'
}], 1);
});

});