set and add functions

[Sozialarchiv]

@kerim1

I thinnk it is a little bit confussion, that some set functions are setting and some adding values.

E.g. setViewingDirectory sets a value and setBehavior adds a value

---

I think with two functions it would be more clear:

setBehavior(behavior?: CollectionBehavior[]) {
  this.behavior = behavior;
}

addBehavior(behavior: CollectionBehavior | CollectionBehavior[]) {
  this.behavior = Base.addArrayValue(behavior, this.behavior);
}

[kerim1]

I agree. Actually it should be set when it is used to set a single property and add when you can call the method repeatedly and it will add the properties to the manifest (not overriding previous added properties).

[Sozialarchiv]

Should I do a pull request? Or do you want to make one?

[Sozialarchiv]

I create a draft:
f1623e5

@kerim1 Can you review it?

Open questions:

1. Should we rename setBehaviour to setBehaviors?
2. In Base there are the elements logo, thumbnail and provider which are in the iiif spec arrays. We allow only single values:

setProvider(provider: Provider): void {
    this.provider = Base.setArrayValue(provider, this.provider);
}

Should we leave it like it is or should we handle it like the other array elements?

[Sozialarchiv]

I recreated the branch.
f1623e5

[kerim1]

Looks really good. To answer your questions:

1. Should we rename setBehaviour to setBehaviors? Let's make that change. Then at least it is consistent with other methods like setItems.
2. In Base there are the elements logo, thumbnail and provider which are in the iiif spec arrays. Should we leave it like it is or should we handle it like the other array elements? Let's leave it like that for now, because there is hardly any use case for multiple values. But then we can always add these methods if the need arises. However, we have to replace the Base.setArrayValue(<value>, <property>) with <property> = [<value>] like the other set methods to keep it consistent. And then we can simply remove the Base.setArrayValue method, because we don't need it any more.

And one small comment: setItems(items: undefined | Canvas[]) is exactly the same as setItems(items?: Canvas[]). But I prefer the latter one, as it is more concise with the ? instead of undefined.

[Sozialarchiv]

Thank you for your feedback,

Should we rename setBehaviour to setBehaviors? Let's make that change. Then at least it is consistent with other methods like setItems.

I will do a commit.

Let's leave it like that for now, because there is hardly any use case for multiple values.

I agree. If we really need it later we can still add e.g. the functions setProviders and addProvider and keep setProvider.

However, we have to replace the Base.setArrayValue(, ) with = [] like the other set methods to keep it consistent. And then we can simply remove the Base.setArrayValue method, because we don't need it any more.

I will do a commit.

And one small comment: setItems(items: undefined | Canvas[]) is exactly the same as setItems(items?: Canvas[]). But I prefer the latter one, as it is more concise with the ? instead of undefined.

Are you sure it is the exact the same? I thought setItems(items?: Canvas[]) means that items is an optional value so the call setItems() is valid. But with setItems(items: undefined | Canvas[]) the call setItems() is invalid. Only the call setItems(undefined) is valid.

Since in our case it is an unset, I thought the second variant is more suitable. I admit, however, that this is a bit sophistical. If you prefer the ? I can gladly change that.

[kerim1]

Are you sure it is the exact the same? I thought setItems(items?: Canvas[]) means that items is an optional value so the call setItems() is valid. But with setItems(items: undefined | Canvas[]) the call setItems() is invalid. Only the call setItems(undefined) is valid.

See https://www.typescriptlang.org/docs/handbook/2/functions.html#optional-parameters

"We can model this in TypeScript by marking the parameter as optional with ?. Although the parameter is specified as type number, the x parameter will actually have the type number | undefined because unspecified parameters in JavaScript get the value undefined."