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

Array edits #1403

Merged
merged 18 commits into from
Feb 24, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
47 changes: 31 additions & 16 deletions src/lib/nested_property.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,20 +116,36 @@ function npGet(cont, parts) {

/*
* Can this value be deleted? We can delete any empty object (null, undefined, [], {})
* EXCEPT empty data arrays. Info arrays can be safely deleted, but not deleting them
* has no ill effects other than leaving a trace or layout object with some cruft in it.
* But deleting data arrays can change the meaning of the object, as `[]` means there is
* EXCEPT empty data arrays, {} inside an array, or anything INSIDE an *args* array.
*
* Info arrays can be safely deleted, but not deleting them has no ill effects other
* than leaving a trace or layout object with some cruft in it.
*
* Deleting data arrays can change the meaning of the object, as `[]` means there is
* data for this attribute, it's just empty right now while `undefined` means the data
* should be filled in with defaults to match other data arrays. So we do some simple
* tests here to find known non-data arrays but don't worry too much about not deleting
* some arrays that would actually be safe to delete.
* should be filled in with defaults to match other data arrays.
*
* `{}` inside an array means "the default object" which is clearly different from
* popping it off the end of the array, or setting it `undefined` inside the array.
*
* *args* arrays get passed directly to API methods and we should respect precisely
* what the user has put there - although if the whole *args* array is empty it's fine
* to delete that.
*
* So we do some simple tests here to find known non-data arrays but don't worry too
* much about not deleting some arrays that would actually be safe to delete.
*/
var INFO_PATTERNS = /(^|.)((domain|range)(\.[xy])?|args|parallels)$/;
var INFO_PATTERNS = /(^|\.)((domain|range)(\.[xy])?|args|parallels)$/;
var ARGS_PATTERN = /(^|\.)args\[/;
function isDeletable(val, propStr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work for domain.x and domain.y ?

Copy link
Contributor

Choose a reason for hiding this comment

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

And more generally, shouldn't all info_array attributes (e.g. updemenues/sliders args, and geo.projection.parallels) be deletable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good point. I was trying to match the previous logic and extend it gently but honestly I can't recall anymore why we had that logic - why does an empty data array need to stick around while other empty arrays can be removed entirely? Perhaps just to keep things simple for streaming? Does that point to a more robust condition on what should be deletable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, letting every array be deletable and running the full test suite was fairly enlightening - There are a lot of things that fail or error out, some of which could be addressed but fundamentally for data arrays there really is a difference between [] (which means there is data of that kind, it's just presently empty) and undefined (which means the data/attribute should get some default value filled in).

As far as I can tell, for info_array attributes there is no difference between [] and undefined though. So unless we want to try and build in here some way to hunt through the schema (which sounds super painful to me, and potentially not even well-defined given that nestedProperty can, on occasion, be called on a root object that's neither a complete trace or layout object but some portion of it) I would argue for some simple tests that might frequently describe an info array as falsely not deletable but will never describe a real data array as deletable.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, for info_array attributes there is no difference between [] and undefined though

That's correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

expanded the safe-to-delete list to include all info_array attributes in 1af058a

@rreusser this also required making args optional for sliders and updatemenus (fallback to [] in command.js) since an empty array will now get pruned. Not sure there will be a real use case for this (what method could one call with no args?) but this case pops up in the tests anyhow 🏆 and it does seem more plotly-esque to not require empty parameters.

Copy link
Contributor

@rreusser rreusser Feb 23, 2017

Choose a reason for hiding this comment

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

The only use case I'm aware of is animating an empty list of frames [] which (serendipitously) functions as a pause button. I've had a bit of trouble with this behavior, so we documented pretty clearly in the web docs that you must use [null] instead of [] to prevent it from getting filtered and replaced with a bare null.

That is, in particular, it's when you create an updatemenus button with a command that pauses. It filters updatemenus[0].buttons[0].args = [[], {...}] and replaces it with [null, {...}] in that case.

if(!emptyObj(val)) return false;
if(!emptyObj(val) ||
(isPlainObject(val) && propStr.charAt(propStr.length - 1) === ']') ||
(propStr.match(ARGS_PATTERN) && val !== undefined)
) {
return false;
}
if(!isArray(val)) return true;

// domain and range are special - they show up in lots of places so hard code here.
if(propStr.match(INFO_PATTERNS)) return true;

var match = containerArrayMatch(propStr);
Expand Down Expand Up @@ -186,8 +202,11 @@ function npSet(cont, parts, propStr) {
}

function joinPropStr(propStr, newPart) {
if(!propStr) return newPart;
return propStr + isNumeric(newPart) ? ('[' + newPart + ']') : ('.' + newPart);
var toAdd = newPart;
if(isNumeric(newPart)) toAdd = '[' + newPart + ']';
else if(propStr) toAdd = '.' + newPart;

return propStr + toAdd;
}

// handle special -1 array index
Expand Down Expand Up @@ -244,11 +263,7 @@ function pruneContainers(containerLevels) {
remainingKeys = false;
if(isArray(curCont)) {
for(j = curCont.length - 1; j >= 0; j--) {
// If there's a plain object in an array, it's a container array
// so we don't delete empty containers because they still have meaning.
// `editContainerArray` handles the API for adding/removing objects
// in this case.
if(emptyObj(curCont[j]) && !isPlainObject(curCont[j])) {
if(isDeletable(curCont[j], joinPropStr(propPart, j))) {
if(remainingKeys) curCont[j] = undefined;
else curCont.pop();
}
Expand Down
24 changes: 24 additions & 0 deletions test/jasmine/tests/lib_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,30 @@ describe('Test lib.js:', function() {
expect(obj).toEqual({a: [undefined, {}]});
});

it('does not prune inside `args` arrays', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done. Thanks!

var obj = {},
args = np(obj, 'args');

args.set([]);
expect(obj.args).toBeUndefined();

args.set([null]);
expect(obj.args).toEqual([null]);

np(obj, 'args[1]').set([]);
expect(obj.args).toEqual([null, []]);

np(obj, 'args[2]').set({});
expect(obj.args).toEqual([null, [], {}]);

np(obj, 'args[1]').set();
expect(obj.args).toEqual([null, undefined, {}]);

// we still trim undefined off the end of arrays, but nothing else.
np(obj, 'args[2]').set();
expect(obj.args).toEqual([null]);
});

it('should get empty, and fail on set, with a bad input object', function() {
var badProps = [
np(5, 'a'),
Expand Down