forio / contour

Apache License 2.0
120 stars 21 forks source link

Materialize configuration options parameters that are functions. #263

Closed jaimedp closed 6 years ago

jaimedp commented 7 years ago

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.

jaimedp commented 6 years ago

I think this is ready to be merged?

narenranjit commented 6 years ago

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

jaimedp commented 6 years ago

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

narenranjit commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 notifications@github.com 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 https://github.com/forio/contour/pull/263#issuecomment-349819466, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVimeL2ECjWdiKu9ogcKi6XRmLL37Wiks5s9y-jgaJpZM4Qc49_ .

narenranjit commented 6 years ago

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

jaimedp commented 6 years ago

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 notifications@github.com 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 https://github.com/forio/contour/pull/263#issuecomment-349822898, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVimc1KtrPMe3HuJnOuDnzpRd1f9B8Gks5s9zPwgaJpZM4Qc49_ .

jaimedp commented 6 years ago

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