Skip to content

Conversation

@VeraZab
Copy link
Contributor

@VeraZab VeraZab commented May 3, 2018

@bpostlethwaite would you 💃 ?
This adjusts the dereferencing function so as to remove the parent[key] and parent[keysrc] from dereferenced object, if dataSources is an empty object.

It fixes the case in streambed where the plot keeps its data, even if the column referenced in it has been removed.

@bpostlethwaite
Copy link
Member

bpostlethwaite commented May 3, 2018

Perhaps we should lift the dereference function from editor and add it to a utility in streambed then we can extend it with impunity to fit the streambed use cases?

When will dataSources be an empty object? What happens in the case below?

dataSources = {
   a: [1,2,3]
}
data = [{xsrc: 'b', x: [1,1,1], ysrc: 'a', y: [2,2,2]}]
dereference(data, dataSources)

@VeraZab
Copy link
Contributor Author

VeraZab commented May 3, 2018

In streambed, dataSources are an empty object when none of the grids have a used column (all columns are empty).

Yeah I think that instead of looking for an empty dataSources object, I should be looking if data is defined, if it isn't, then delete the keys.

    if (!Array.isArray(data)) {
      if (!data) {
        delete parent[dataKey];
        delete parent[key];
      }
      return;
    }

maybe even just:

      if (!data || !Array.isArray(data)) {
        delete parent[dataKey];
        delete parent[key];
        return;
      }

Would there be a situation where data is not an array but we don't want to delete the key from the parent?

@bpostlethwaite
Copy link
Member

bpostlethwaite commented May 3, 2018

Would there be a situation where data is not an array but we don't want to delete the key from the parent?

Not 100% sure. I don't think we should be too prescriptive here. What about checking for key existence instead of the data type of the dataSource value?

Hmmm if we delete the src reference from the figure we are losing information about what data the figure is asking for. Just because we don't have that data when dereference is called doesn't mean we won't have it at some other time. What is the case for deleting it?

Also if we put deletion into dereference we can't pass in partial dataSources. I remember relying on that behaviour before. We could put this deletion behind an option flag or make a new function like syncFigure(figure, dataSources).

@VeraZab
Copy link
Contributor Author

VeraZab commented May 3, 2018

What about checking for key existence instead of the data type of the dataSource value?

yeah, that is better. We've spoken briefly with Nic about the possibility of loading in json templates of a figure, with srcs instead of values, and I think its a good idea, so true, removing srcs is maybe not what we'd like to do.

We could put this deletion behind a option flag

I think that's reasonable, I'm going to do that. Thanks!

@VeraZab
Copy link
Contributor Author

VeraZab commented May 3, 2018

ok @bpostlethwaite I adjusted it

const SRC_ATTR_PATTERN = /src$/;

export default function dereference(container, dataSources) {
export default function dereference(container, dataSources, deleteKeys) {
Copy link
Member

@bpostlethwaite bpostlethwaite May 3, 2018

Choose a reason for hiding this comment

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

I would prefer this to be a config field so we can add arbitrary functionality later on without needing to mess with positional parameters

dereference(container, dataSources, config = {})

Copy link
Member

@bpostlethwaite bpostlethwaite left a comment

Choose a reason for hiding this comment

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

Ok looks good! 💃

@VeraZab VeraZab force-pushed the adjust-dereferencing branch from c7b3cfd to e5886e2 Compare May 3, 2018 15:27
@VeraZab VeraZab merged commit 684ec5b into master May 3, 2018
@VeraZab VeraZab deleted the adjust-dereferencing branch May 3, 2018 15:32
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

Successfully merging this pull request may close these issues.

3 participants