fiverr / passable

Declarative data validations.
https://fiverr.github.io/passable/
MIT License
100 stars 11 forks source link

Add `someOf` runner for "compound enforces" #51

Closed ealush closed 5 years ago

ealush commented 6 years ago

Carrying on from https://github.com/fiverr/passable/issues/20.

Instead of abusing the anyOf runner in order to support compound enforce, we can instead allocate a new one, with its own syntax.

As described in #20: Support a way to group .anyOf() conditions

Currently we support these three main enforce conditionals: noneOf(), allOf(), anyOf().

With anyOf we can easily determine whether a single condition is true, so we can do things like:

  • may either be a string or a number
    enforce(value).anyOf({
    isString: true,
    isNumber: true
    });
  • may be empty or larger than 55
    enforce(value).anyOf({
    isEmpty: true,
    largerThan: 55
    });

But there is no straightforward way to test against groups of conditionals, for example:

  • May be an array or a number smaller than 25

Multiple ideas were suggested (thanks @ronen-e), but none of the suggestion solutions could provide both ease of use and readability/style. I now believe it is time to support a fourth runner: someOf

Why someOf

Similar to anyOf, someOf hints that we're looking for at least one of an unspecified amount of items to pass the test.

What would the syntax be?

Currently, the most fitting syntax in my opinion would be per-argument-evaluation, meaning:

enforce(data.value).someOf({
    isArray: true,
    isEmpty: false
}, {
    isNumber: true,
    largerThan: 55
});

In the example above, we allow data.value to either be a non-empty-array or a number larger than 55.

ronen-e commented 6 years ago

I think this would cause confusion with regards to the API, at least name wise. It's not intuitive to differentiate between anyOf and someOf. They have similar meaning.

I would choose a different name or extend anyOf to support that logic.

ealush commented 6 years ago

@ronen-e, I prefer not to alter the functionality of anyOf, due to the, mainly for fact that it is bound to confuse consumers.

I came up with someOf as it does express the notion of grouping things together. Do you have another idea that represents this concept?


One suggestion made by @omrilotan is to create modifier functions that will mark the changing behavior of a function, for example .compound:

enforce(...).compound().anyOf({...}, {...}, {...});

which is fairly simple to implement, but might cause its own set of problems. How far down the chain does it apply to, etc.

ronen-e commented 6 years ago

@ealush I made suggestions in #20 regarding this concept.

  1. using arrays
  2. using symbols

Your idea of using multiple parameters is also fine.

I personally do not think it would confuse consumers to extend the functionality of anyOf.

Also, you should take into account the possibility of adding the same functionality to allOf and noneOf

For example, let's say you have a form with an age field and you decide the following rules:

It would be intuitive to use noneOf:

enforce(age).noneOf(
 { largerThanOrEquals: 25, smallerThanOrEquals: 30 },
 { largerThanOrEquals: 40, smallerThanOrEquals: 45 }
)

The alternative, with someOf, would be less intuitive and more confusing:

enforce(age).someOf(
 { smallerThan: 25, largerThan: 45 },
 { largerThanOrEquals: 30, smallerThanOrEquals: 40 }
)

But if you would rather create a new method (like with someOf) then you would have to do the same for the use case mentioned above (nothingOf maybe?)


being able to extend enforce could be a nice addition. something like:

enforce(...).compound()
// or
enforce(...).extend(options)

But I do not think it is necessary right now. I don't know of any specific use cases, except for the current issue, which may be solved using the solution I explained.

ealush commented 6 years ago

@ronen-e, great inputs. I agree that adding nesting and symbols as a feature could be beneficial, but in my opinion it is a different issue. Same goes for allowing nested  runners.


But if you would rather create a new method (like with someOf) then you would have to do the same for the use case mentioned above (nothingOf maybe?)

I believe there's a clear distinction between && and || tests in this regard.

Regarding your noneOf example:

enforce(age).noneOf(
 { largerThanOrEquals: 25, smallerThanOrEquals: 30 },
 { largerThanOrEquals: 40, smallerThanOrEquals: 45 }
)

This highly emphasizes my intention. Extending the syntax to support multiple arguments would be very clear in && enforcements (such as allOf and noneOf), as there is no confusion to which the && applies to. It simply applies to everything. The syntax can be easily added && without confusing consumers.

All the following practically mean the same thing:

// future syntax
enforce(age).noneOf(
 { largerThanOrEquals: 25, smallerThanOrEquals: 30 },
 { largerThanOrEquals: 40, smallerThanOrEquals: 45 }
);
===
// current syntax (chaining)
enforce(age)
    .noneOf({largerThanOrEquals: 25, smallerThanOrEquals: 30})
    .noneOf({largerThanOrEquals: 40, smallerThanOrEquals: 45})
===
// js representation
(!(age >= 25 && age <= 30) && !(age >= 40 && age <= 45))

But the same cannot be easily assumed for || tests (anyOf), as I cannot intuitively tell if my any condition is between the test objects, in each object respectively.

The following:

enforce(age)
    .anyOf(
        { largerThanOrEquals: 25, smallerThanOrEquals: 30 },
        { largerThanOrEquals: 40, smallerThanOrEquals: 45 }
    )

Could mean any of the following:

// Let's assume it is logically correct. I am just using your example

((age >= 25 || age <= 30) && (age >= 40 || age <= 45))
// OR
((age >= 25 || age <= 30) || (age >= 40 || age <= 45))

And I could not tell from the name or the syntax of the function, which of these evaluations will be performed. Which would you assume it does?

Using someOf won't reveal that too, but it will indicate the fact that this other function has its own behavior which is different from all the rest.


Of course, it may be a thing that can be solved by adding proper documentation to the new syntax - but this thread is not about documentation, but about finding the most intuitive syntax that wouldn't require the consumer to keep going back the documentation.

ronen-e commented 6 years ago

@ealush I think I am not following your logic.

In #20 you wrote:

But there is no straightforward way to test against groups of conditionals, for example:

  • May be an array or a number smaller than 25

Initially I thought of the following syntax

enforce(value).anyOf({
isArray: true
}, {
isNumber: true,
smallerThan: 25
});

So, I was under the impression that each argument is a set which is checked using the AND condition.

Meaning, each set is checked for All the rules in the set and then checked according to the logic of the specific test runner (i.e anyOf validates at least one set of rules, allOf validates all sets of rules)

So in the example:

enforce(age)
    .anyOf(
        { largerThanOrEquals: 25, smallerThanOrEquals: 30 },
        { largerThanOrEquals: 40, smallerThanOrEquals: 45 }
    )

I believed it stands for:

(age >= 25 && age <= 30) || (age >= 40 && age <= 45)
ealush commented 6 years ago

You are correct. I forgot to add the THIRD case which this syntax could be representing.

((age >= 25 || age <= 30) && (age >= 40 || age <= 45))
// OR
((age >= 25 || age <= 30) || (age >= 40 || age <= 45))
// OR -> my initial intention
((age >= 25 && age <= 30) || (age >= 40 && age <= 45))

-> Which further illustrates my unwillingness to extend the current function. It really is confusing IMO.

ealush commented 5 years ago

Compound rules will not be supported in v7.