Materialize configuration options parameters that are functions. #263

jaimedp · 2017-11-14T07:04:31Z

This allows to specify any configuration option as a function. This functions will be called just before the rendering starts. Some functions are not "materialized" as they expect to be called with some data within the context of the render cycle for example data formatters expect to be called for each point in the data set. These functions are not materialized before the render starts and are called in the context of the specific visualization.

narenranjit · 2017-11-15T20:28:19Z

src/scripts/core/contour-utils.js

+                    var curKeyPath = curPath ? curPath + '.' + key : key;
+                    var notInSkipList =  skipList.indexOf(curKeyPath) === -1;
+                    var notSkipMatch = (skipMatch && skipMatch.test ? !skipMatch.test(curKeyPath) : true);
+                    var expectsParam = /(function)?\s?\(.+\)\s*({|=>)/.test((object[key] || '').toString());


I think you can just do object[key].length to get how many parameters it takes.

Why do we need to separate functions which specify params? For the ones which matter, like the formatter, can't we just blacklist them with the skip lists? Seems a bit scary to think marker : { enable: ()=> true } will work but marker : { enable: (randomCopyPasteParameter)=> true } won't.

I was thinking the same thing... But otherwise, for regular callback functions like formatters, you have to remember to add the skip list which would not be very intuitive. We could leave this option out if you want.

You mean "you" as in "Contour developer", not "end user" right? i.e., we'd already know what to blacklist and it'll only be a problem if we wanted to add a new Contour options down the line which took in a function-with-param value, in which case we'd have to add it to the blacklist.

Or would you need to use this when you're building a custom visualization?

I think this impacts two groups:

contour core developers, when adding new functionality. I guess this is less of a problem since you "we know what we are doing" :)

Contour lib users. If you are developing a custom visualization that calls a function defined in your part of the config object, you'll need provide a default skip list that includes your function, but if the user of your visualization overrides the skip list, then the viz can break without much indication as of why. For example, let's say I create a custom viz like this:

const renderer = (data, layer, options) => { layer.selectAll('.translations').data(data); layer.enter() .append('div') .text(options.translate.getTranslation); }; renderer.defaults = { translate: { getTranslation: (d) => `data is ${d.x}, ${d.y}`, }, skip: ['translate.getTranslation'] } Contour.export('translate', renderer);

in this case, if the user of the visualization passes a different skip list, translate viz would break.

Makes sense? maybe we can force to the functions to have a specific signature to prevent them from being materialized, but again consumers of custom viz would have to remember to follow the signature when passing a fn param.

In general my thinking was that the case were you pass a function that has a parameter by mistake is less probable than the case where you need to remember to add it to the skip list.

Another idea could be to have a "debug" mode in contour where you get debugging/console messages that tell you which functions got materialized.

That makes sense - how about a whitelist of 'known materializable properties' instead of a blacklisted skip list then? This would mean there is no change in behavior for developing custom viz, unless we allow them to 'opt-in' by passing in an 'allow' flag (or maybe we just don't expose this to custom viz).

That said, you know a lot more about the different cases than I do so I'm fine going with your call either way.

I thought about going with the whitelist route too, but then that would mean that mostly everything would be in the whitelist (as most props are fine to have functions) and if I create a custom viz, I need to add each property that can be materializable to the list except from my little lonely getTranslations function... that seem an overkill too :)

I meant whitelist specifically for custom visualizatons; if i'm defining a custom viz which takes in a boolean prop I wouldn't automatically expect that to be callable with a function -- or does that distinction between external/internal visualizations not exist?

There not really a concept of internal vs external, unless we keep an internal list... as long as you have access to the Contour object, you can export new viz to be used by anyone.

narenranjit · 2017-11-15T20:34:33Z

src/documentation/config/config.md

@@ -38,6 +38,12 @@ Each configuration option can be:

 		columnWidth: function() { return this.rangeBand/3 * 2; }

+	__Note__ Function parameters will be invoked just before each render happens or in the within the render cycle with the corresponding data point in the following cases:


do you mean except the following cases, or am i mis-understanding what this is?

I changed the text a bit to make it clearer

narenranjit · 2017-11-15T20:38:35Z

src/documentation/config/config.md

+	__Note__ Function parameters will be invoked just before each render happens or in the within the render cycle with the corresponding data point in the following cases:
+	* function expects a parameter (ie. `function (d) { return d.x; }`)
+	* parameter path is in the `options.skip`
+	* parameter path matches the RegExp `options.skipMatch`


Since you're computing the "path" anyway, in which cases would just the skip array not be good enough?

I change the skip array to accept full paths, or regexp so you have a simple list of things to skip. I think the regexp is handy to skip global params like formatter (ie /formatter/) without having to specify every path that you want to skip.

jaimedp · 2017-11-16T18:11:37Z

src/scripts/core/contour.js

@@ -315,7 +315,7 @@
            _.each(this._extraOptions, mergeExtraOptions);
            _.each(this._visualizations, mergeDefaults);

-            var opt = _.extend({}, this.originalOptions);
+            var opt = _.nw.materialize(this.originalOptions, this, { skipMatch: /formatter/ });


Note to self: this needs to be updated to remove param skipMatch and merge the original skip list

var opt = _.nw.materialize(this.originalOptions, this, { skip: [/formatter/].concat(this.originaOptions.skip) });

jaimedp · 2017-11-16T18:15:53Z

tests/specs/contour-utils-spec.js

+
+            it('should skip functions that expect parameters', function () {
+                expect(res.d.other).toEqual(input.d.other);
+            });


note to self:
Add test to check default call to materialize always includes forces ignore props (ie. /formatter/)

jaimedp · 2017-12-01T07:59:39Z

I think this is ready to be merged?

narenranjit · 2017-12-01T15:19:13Z

Sorry, I thought your "note to self" meant you were going to add something else :) But now this has conflicts, can you resolve?

jaimedp · 2017-12-05T05:39:45Z

I added the test that the note-to-self refer to :) I rebase and has no more conflicts now.

narenranjit · 2017-12-06T19:46:58Z

@jaimedp I did a quick hallway usability test for materialize as a function.

I asked someone to write code to toggle markers based on a checkbox, and they came up with

enable: ()=> chk.checked;

I then asked them to only show markers if data.length > 10, and they started with

enable: (a, b)=> console.log(a, b);

to see if the data is passed in as an parameter, and of course, that doesn't work and it's impossible to guess why not.

Admittedly sample size of 1, and you could argue calling this out prominently in the docs would mitigate this, but I think we shout at least console.warn if it's a function and it's not parsed, or whitelist. What do you think?

jaimedp · 2017-12-06T23:50:37Z

Why would you expect to receive x, y parameters in that function? Nothing else receives x,y ... they receive either the series or the data point object like formatters, but that's expected for something that I need to decide or return a value per data point... to enable/disable markers, I would expect that I have to decide once per chart... right?

Maybe we can introduce a debug mode where it tells you what functions are going to be materialized?

narenranjit · 2017-12-07T00:21:55Z

x, y was just an example -- i.e. random parameter names to test out what data is getting passed into a function, except the act of figuring it out makes it invalid. A new variation of a heisenbug :) You could do always do console.log(arguments) of course.

Why not log what parameters are functions and are not materialized all the time? i.e. something like if isFunction(object[key]) && object[key].length && !['formatter', ...].contains(key)

jaimedp · 2017-12-07T00:35:07Z

It could be, I just don't think we should be polluting the console with messages when most of the time are not useful, besides it actually could make the `materialize` call slower

…

On Wed, Dec 6, 2017 at 4:21 PM, Narendran Ranjit ***@***.***> wrote: x, y was just an example -- i.e. random parameter names to test out what data is getting passed into a function, except the act of figuring it out makes it invalid. A new variation of a heisenbug :) You could do always do console.log(arguments) of course. Why not log what parameters are functions and are *not* materialized all the time? i.e. something like if isFunction(object[key]) && object[key].length && !['formatter', ...].contains(key) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#263 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVimeL2ECjWdiKu9ogcKi6XRmLL37Wiks5s9y-jgaJpZM4Qc49_> .

narenranjit · 2017-12-07T00:40:15Z

In what cases will there be a false-positive warning?

jaimedp · 2017-12-07T00:48:12Z

I see, you mean only warn if you have a fn that expects parameters that is not whitelisted in the skip list... I think that could work.

…

On Wed, Dec 6, 2017 at 4:40 PM, Narendran Ranjit ***@***.***> wrote: In what cases will there be a false-positive warning? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#263 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVimc1KtrPMe3HuJnOuDnzpRd1f9B8Gks5s9zPwgaJpZM4Qc49_> .

jaimedp · 2017-12-07T06:03:22Z

I added a warning when you have a function with params that is not in the skip list

narenranjit self-assigned this Nov 15, 2017

narenranjit suggested changes Nov 15, 2017

View reviewed changes

jaimedp force-pushed the fn-in-options branch 2 times, most recently from fa1df30 to 874e004 Compare November 16, 2017 06:09

jaimedp commented Nov 16, 2017

View reviewed changes

jaimedp force-pushed the fn-in-options branch from 874e004 to f76c95f Compare November 17, 2017 06:25

Materialize configuration options parameters that are functions.

fd089b1

jaimedp force-pushed the fn-in-options branch from f76c95f to fd089b1 Compare December 5, 2017 05:38

Add warn message when config fn expects parameters

cb04b37

narenranjit approved these changes Dec 7, 2017

View reviewed changes

narenranjit merged commit 81b9840 into forio:master Dec 7, 2017

jaimedp deleted the fn-in-options branch January 24, 2018 05:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Materialize configuration options parameters that are functions. #263

Materialize configuration options parameters that are functions. #263

jaimedp commented Nov 14, 2017

narenranjit Nov 15, 2017

narenranjit Nov 15, 2017

jaimedp Nov 16, 2017

narenranjit Nov 16, 2017 •

edited

jaimedp Nov 16, 2017

narenranjit Nov 16, 2017

jaimedp Nov 16, 2017

narenranjit Nov 16, 2017 •

edited

jaimedp Nov 16, 2017

narenranjit Nov 15, 2017

jaimedp Nov 16, 2017

narenranjit Nov 15, 2017

jaimedp Nov 16, 2017

jaimedp Nov 16, 2017

jaimedp Nov 16, 2017

jaimedp commented Dec 1, 2017

narenranjit commented Dec 1, 2017

jaimedp commented Dec 5, 2017

narenranjit commented Dec 6, 2017 •

edited

jaimedp commented Dec 6, 2017 •

edited

narenranjit commented Dec 7, 2017

jaimedp commented Dec 7, 2017 via email

narenranjit commented Dec 7, 2017

jaimedp commented Dec 7, 2017 via email

jaimedp commented Dec 7, 2017

		@@ -38,6 +38,12 @@ Each configuration option can be:

		columnWidth: function() { return this.rangeBand/3 * 2; }

		__Note__ Function parameters will be invoked just before each render happens or in the within the render cycle with the corresponding data point in the following cases:

Materialize configuration options parameters that are functions. #263

Materialize configuration options parameters that are functions. #263

Conversation

jaimedp commented Nov 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narenranjit Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narenranjit Nov 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaimedp commented Dec 1, 2017

narenranjit commented Dec 1, 2017

jaimedp commented Dec 5, 2017

narenranjit commented Dec 6, 2017 • edited

jaimedp commented Dec 6, 2017 • edited

narenranjit commented Dec 7, 2017

jaimedp commented Dec 7, 2017 via email

narenranjit commented Dec 7, 2017

jaimedp commented Dec 7, 2017 via email

jaimedp commented Dec 7, 2017

narenranjit Nov 16, 2017 •

edited

narenranjit Nov 16, 2017 •

edited

narenranjit commented Dec 6, 2017 •

edited

jaimedp commented Dec 6, 2017 •

edited