Unify getting limit values and checking them

[jgoizueta]

Now a function Checks.check handles both tasks allowing for
multiple checks (checking functions) and multiple limits per check.

See #220

Now, in node classes we'll typically need something like this to check limits:

var LIMITS = {
    maximumNumberOfRows: {
        default: 1e6,
        name: 'maximumNumberOfRows',
        message: 'too many source rows'
    }
};

MyNodeClass.prototype.checkLimits = function(context, callback) {
     Checks.check(this, context, [{
         checker: function(node, context, limitValues, params, done) {
            // this function receives limit values and should check them for the node,
            // calling done(error) if exceeded;
            // The Checks module has some functions like `Checks.limitNumberOfRows`
            // that can be used directly here
         },
         params: { input: 'source' }, // parameters passed to the checker
         limits: LIMITS                      // limits definitions
     }], callback);
 };

The Checks.check functions helps with the process of getting limit values from the configuration and calling checker functions. An array of objects that define each checker is passed.

Using one of the predefined checker functions (limitNumberOfRows, which doesn't require additional parameters) the above example would be:

MyNodeClass.prototype.checkLimits = function(context, callback) {
    Checks.check(this, context, [{
        checker: Checks.limitNumberOfRows,
        limits: LIMITS
    }], callback);
};

[jgoizueta]

This is a remnant of a previsous iteration. I think I'll remove it, since it's not needed now

[rochoa]

If possible, I would remove it now.

[jgoizueta]

I'll do

[jgoizueta]

@rochoa care to take a look? Current functionality isn't change but adding or change limit checks should be easier now.

[rochoa]

Why do we use async here?

[jgoizueta]

No need really, at some point in writing here I was expecting that getting limit values could eventually need some async operation, but now I see no reason for this. I'll remove it. My only doubt is whether to maintain this limitValues function as async (with callback) or not...

[rochoa]

As this method must be overridden, can we document what is the signature for the callback?

[jgoizueta]

sure

[rochoa]

Do we want to have that default limit? Or should it be implicit?

I mean: could we just return callback(null);?

[jgoizueta]

Having this here allow us to limit the number of result rows for any analysis type by setting a limit value in the configuration.

If there's no configuration value, since where have defaultLimit here the behaviour is equivalent to callback(null) (because limitNumberOfRows does exactly that for null/Infinite limit values).

[rochoa]

Isn't it a bit weird that the Checks.check function receives a reference to a function of the same module? I have the feeling this is still very _DSL_ish approach.

Why not something like:

this.limits.isWithinRowsLimit(this.source, limits.maximumNumberOfRows.default, callback)

[rochoa]

Where limits has already configuration limits, that could be injected from outside, to override the default limit provided here.

[jgoizueta]

The reason for the duality (Checks.check & Checks.limitInputRows) and the DSLish approach here is that Checks.check simplifies the processing if we want to combine multiple separate checks (e.g. check input rows and output rows), and handling for each separate check its limits values.

Then, for each check passed to Checks.check, the user can supply a checker function or use a predefined one such as Checks.limitInputRowshere.

I see your point with the more object-oriented approach you propose, but I think we would miss the ability to easily have multiple checks. (which may not be important really, I'm not sure... so far we're not needing it).

[jgoizueta]

What I like about this.limits.checkingOp(...) vs Checks.check(..., [{...definitions...}]) is how it simplifies some things (we don't need a check for input rows, and passing the input parameter since we have a check for output rows).

[jgoizueta]

It should be var limitName = limitParams.name || limit; shouldn't it?