master-modifier-contains-nby

[Goaman]

No description provided.

[Zynton]

Du coup pour rester dans l'esprit des autres méthodes, ce serait bien d'aussi accepter ...modifiers: Array<Modifier | Constructor<Modifier>>.

[Goaman]

I specifically need to check if Modifiers are contained, not Modifier.

Including Constructor<Modifier> would introduce a new logic that I think is useless. Modifier.isSameAs does not take Constructor<Modifier>.
If we need to check the constructor, then using a .filter() would be sufficient as we don't need to check the undefined.

On the other hand I could use Modifier[] in the signature. Which could have two forms:

    contains(modifiers: Modifiers | Modifier[]): boolean {
        //...

or:

    contains(...args: [Modifiers] | Modifier[]): boolean {
        const modifiers = args[0] instanceof Modifiers ? args[0] : (args as Modifier[]);
        //...

Which one you prefer @dmo-odoo @Zinston ?

I've push the first version as I found it more elegant but I'll change it if you have a different taste.

[Zynton]

The reason I suggested to include the other types was that most methods in Modifiers do and that this one is not fundamentally different from the others so from an API perspective I would expect to be able to use this one the same was as the others. Even though it admittedly adds a little bit of logic. As you said though:

If we need to check the constructor, then using a .filter() would be sufficient as we don't need to check the undefined.
So there's little cost to putting that in contains.

[Goaman]

So what do I do?
Do I continue to take Modifiers as argument?
Do I add the logic for Contructor ?

If I take it all it means the function would become this:

    contains(...args: [Modifiers] | (Modifier | Constructor<Modifier>)[]): boolean {
        if (args[0] instanceof Modifiers || args[0] instanceof Modifier) {
            const modifiers =
                args[0] instanceof Modifiers
                    ? args[0]
                    : (args as (Modifier)[]);
            for (const modifier of modifiers) {
                const foundModifier = this.find(m => m.isSameAs(modifier));
                // Modifier.isSameAs(undefined) may return true in the case of
                // modifiers that are themselves containers of values. Then we might
                // want to equate the presence of an "empty" modifier with the
                // absence of that modifier.
                if (!foundModifier && !modifier.isSameAs(undefined)) {
                    return false;
                }
            } else {
                const modifierClasses = [...new Set(args as Constructor<Modifier>[])];
                return modifierClasses.every(this.find.bind(this))
            }
        }
        return true;
    }

Which becomes complicated to read for not so much value.
And also if I want consistencies, it means that all other method should have the same signature (i.e. including Modifiers).

[Goaman]

To summarize some of the options:
...modifiers: (Modifier | Constructor<Modifier>)[]

• I don't like because it does not take Modifiers
• add branching + logic for Contstructor

modifiers: Modifiers | Modifier[]

• +simple
• require to change the other signature for consistency
• add branching + logic for Contstructor

modifiers: Modifiers | (Modifier | Constructor<Modifier>)[]

• require to change the other signature for consistency
• add branching + logic for Contstructor

...args: [Modifiers] | (Modifier | Constructor<Modifier>)[]

• require to change the other signature for consistency
• add branching + logic for Contstructor

[Zynton]

👍
[...modifiers] would magically call Modifiers.clone ?

[Goaman]

I've tested I it does not work. You need f(...[...modifier]).
Then the code would be:

    contains(...modifiers: Modifier[] | Constructor<Modifier>[]): boolean {
        if (modifiers[0] instanceof Modifier) {
            for (const modifier of modifiers as Modifier[]) {
                const foundModifier = this.find(m => m.isSameAs(modifier));
                // Modifier.isSameAs(undefined) may return true in the case of
                // modifiers that are themselves containers of values. Then we might
                // want to equate the presence of an "empty" modifier with the
                // absence of that modifier.
                if (!foundModifier && !modifier.isSameAs(undefined)) {
                    return false;
                }
            }
            return true;
        } else {
            const modifierClasses = [...new Set(modifiers as Constructor<Modifier>[])];
            return modifierClasses.every(this.find.bind(this));
        }
    }
    areSameAs(otherModifiers: Modifiers): boolean {
        return this.contains(...[...otherModifiers]) && otherModifiers.contains(...[...this]);
    }

[Goaman]

The first ... is the syntax to specify that you want an array being considered as the list of arguments.
So you need to ... over an array.
The second ... mean taking an iterable (here otherModifier) and place it in an array through the spread operator.
I think the first ... is not considered as a spread operator although it is written the same way.

[dmo-odoo]

Mmmh... Then what about this:

contains(...modifiers: Iterable<Modifier | Constructor<Modifier>): boolean {

Wouldn't Modifiers be an Iterable<Modifier> ?

[Goaman]

I already tried. I've tested in javascript and it would work.

The problem is typescript.
If you do fn(...anything: AnyType) {}, typescript tell you AnyType must be an array of something.
An iterable is not an Array so fn(...anything: Iterable<any>) {} cannot work.

And if you do contains(...modifiers) (which would work in js), typescript tells you:

Argument of type 'unknown[]' is not assignable to parameter of type 'Modifier[] | Constructor[]'.

[Zynton]

Attribute est un plugin non ? Je ne le mentionnerais pas dans core. Tu peux peut-être plutôt expliquer pourquoi on peut se retrouver dans cette situation ?

[Goaman]

It's tricky to explain I have removed Attribute.isSameAs. Even though it does not shock me to make a reference to another plugin within a comment in core if it help to understand. Do you have an explanation that comes to your mind?

[Zynton]

How about this?

Modifier.isSameAs(undefined) may return true in the case of modifiers that are themselves containers of values. Then we might want to equate the presence of an "empty" modifier with the absence of that modifier.

[Goaman]

I find it accurate but difficult to understand for someone not familiar. Moreover, technically a modifier that does not represent a container can return true if undefined is provided.

The example of Attribute.isSameAs would give me more information IMO. That's no t a problem to have an external link to explain or provide an example to a concept.

I've changed for your comment anyway.

[Zynton]

Fair enough. I'd do both then.