Skip to content
This repository was archived by the owner on May 5, 2021. It is now read-only.

master-modifier-contains-nby #453

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

master-modifier-contains-nby #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions packages/core/src/Modifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { EventMixin } from '../../utils/src/EventMixin';
import { VersionableArray } from './Memory/VersionableArray';
import { makeVersionable } from './Memory/Versionable';

export class Modifiers extends EventMixin {
export class Modifiers extends EventMixin implements Iterable<Modifier> {
private _contents: Modifier[];
constructor(...modifiers: Array<Modifier | Constructor<Modifier>>) {
super();
Expand Down Expand Up @@ -249,6 +249,20 @@ export class Modifiers extends EventMixin {
toggle(modifier: Modifier | Constructor<Modifier>): void {
this.remove(modifier) || this.append(modifier);
}
/**
* Check that all `otherModifiers` ar contained within this ones.
*
* @param otherModifiers
*/
contains(otherModifiers: Modifiers): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
    }

Copy link
Contributor Author

@Goaman Goaman Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh... Then what about this:

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

Wouldn't Modifiers be an Iterable<Modifier> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[]'.

for (const otherModifier of otherModifiers) {
const foundModifier = this.find(m => m.isSameAs(otherModifier));
if (!foundModifier && !otherModifier.isSameAs(undefined)) {
return false;
}
}
return true;
}
/**
* Return true if the modifiers in this array are the same as the modifiers
* in the given array (as defined by the `isSameAs` methods of the
Expand All @@ -257,17 +271,7 @@ export class Modifiers extends EventMixin {
* @param otherModifiers
*/
areSameAs(otherModifiers: Modifiers): boolean {
const modifiersMap = new Map(
this._contents?.map(a => [a, otherModifiers.find(b => a.isSameAs(b))]) || [],
);
const aModifiers = Array.from(modifiersMap.keys());
const bModifiers = Array.from(modifiersMap.values());

const allAinB = aModifiers.every(a => a.isSameAs(modifiersMap.get(a)));
const allBinA = otherModifiers.every(
b => bModifiers.includes(b) || b.isSameAs(this.find(b)),
);
return allAinB && allBinA;
return this.contains(otherModifiers) && otherModifiers.contains(this);
}
/**
* Remove all modifiers.
Expand Down Expand Up @@ -308,6 +312,20 @@ export class Modifiers extends EventMixin {
map<T>(callbackfn: (value: Modifier, index: number, array: Modifier[]) => T): T[] {
return this._contents?.map(callbackfn) || [];
}
/**
* Iterate through all modifiers.
*/
[Symbol.iterator](): Iterator<Modifier> {
let index = -1;
const data = this._contents || [];

return {
next: (): IteratorResult<Modifier> => ({
value: data[++index],
done: !(index in data),
}),
};
}
/**
* @override
*/
Expand Down
55 changes: 55 additions & 0 deletions packages/core/test/Modifiers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class ExtendedModifier extends Modifier {
return otherModifier instanceof ExtendedModifier && this.value === otherModifier.value;
}
}
class SameUndefinedModifier extends Modifier {
isSameAs(otherModifier: Modifier): boolean {
return otherModifier instanceof Modifier || typeof otherModifier === 'undefined';
}
}
describe('core', () => {
describe('Modifiers', () => {
describe('constructor()', () => {
Expand Down Expand Up @@ -505,6 +510,56 @@ describe('core', () => {
expect(modifiersMap[1] instanceof ExtendedModifier).to.be.true;
});
});
describe('contains()', () => {
it('should contain itself favorably', () => {
const m1 = new Modifier();
const m2 = new Modifier();
const modifiers1 = new Modifiers(m1, m2);
expect(modifiers1.contains(modifiers1)).to.be.true;
});
it('should contain a modifier that has the same favorably', () => {
const m1 = new ExtendedModifier(1);
const m2 = new ExtendedModifier(2);
const modifiers1 = new Modifiers(m1, m2);
const m1bis = new ExtendedModifier(1);
const m2bis = new ExtendedModifier(2);
const modifiers2 = new Modifiers(m1bis);
const modifiers3 = new Modifiers(m2bis);
const modifiers4 = new Modifiers(m1bis, m2bis);
expect(modifiers1.contains(modifiers2)).to.be.true;
expect(modifiers1.contains(modifiers3)).to.be.true;
expect(modifiers1.contains(modifiers4)).to.be.true;
});
it('should contain a modifier that has the same favorably even if their order is different', () => {
const m1 = new ExtendedModifier(1);
const m2 = new ExtendedModifier(2);
const modifiers1 = new Modifiers(m1, m2);
const modifiers2 = new Modifiers(m2, m1);
expect(modifiers1.contains(modifiers2)).to.be.true;
});
it('should contain a modifier that has the same favorably even if their order and instances are different', () => {
const m1 = new ExtendedModifier(1);
const m2 = new ExtendedModifier(2);
const modifiers1 = new Modifiers(m1, m2);
const m1bis = new ExtendedModifier(1);
const m2bis = new ExtendedModifier(2);
const modifiers2 = new Modifiers(m2bis, m1bis);
expect(modifiers1.contains(modifiers2)).to.be.true;
});
it('should match with modifiers that are the same with undefined', () => {
const m1 = new SameUndefinedModifier();
const modifiers1 = new Modifiers();
const modifiers2 = new Modifiers(m1);
expect(modifiers1.contains(modifiers2)).to.be.true;
});
it('should not contain the other modifiers', () => {
const m1 = new ExtendedModifier(1);
const modifiers1 = new Modifiers(m1);
const m2 = new ExtendedModifier(0);
const modifiers2 = new Modifiers(m2);
expect(modifiers1.contains(modifiers2)).to.be.false;
});
});
describe('areSameAs()', () => {
it('should know that an instance of Modifiers is the same as itself', () => {
const m1 = new Modifier();
Expand Down