Add intoAlways overload that accepts a list of diffusers#15
Add intoAlways overload that accepts a list of diffusers#15zvonicek merged 2 commits intospotify:masterfrom
Conversation
| /// This function is useful as a building block for more complex Diffusers, but it is unlikely that you would use | ||
| /// it on its own. Consider using `into` instead. |
There was a problem hiding this comment.
Is this sentence still relevant in those two new variants of intoAlways?
There was a problem hiding this comment.
Also, perhaps you could document briefly the intended use case of intoAlways for non-Equatable models with Equatable members.
There was a problem hiding this comment.
Updated! I think that sentence would still apply in this case but it's not really that clear so I replaced it to point out about the existence of intoAll instead.
zvonicek
left a comment
There was a problem hiding this comment.
One trade-off of this PR is that when the base model is Equatable, there are now two ways of expressing the identical logic, by using .intoAlways or .intoAll. Ideally, I would like the library to be more opinionated and prevent such a state, but I understand that there are limited ways of doing so at the moment without causing a BC break.
I, on the other hand, quite like the .intoAlways solution for dealing with non-Equatable base models, which seems like an elegant way to incorporate this much needed capability into the library.
So I think that we can go ahead with merging this one!
|
cc @togi in case you have any thoughts on this before we merge it |
I get the point but I don't think it's any different from |
A common pattern is to create a
Diffuserthat is a combination of several diffusers that map individual properties of the model:This works fine except that it forces you to make the model
EquatablebecauseintoAllrequires the object to beEquatable. When using this pattern, you don't care about the model beingEquatablebut only about the individual properties beingEquatable, in fact the entire object will be cached byintoAlland then each property will be cached byinto.To avoid this unnecessary conformance to
Equatable, this PR introduces a variant ofintoAlwaysthat accepts a list of diffusers and always run them making the previous example:Since the individual properties are still
Equatableas required by.into, the two examples above are pretty much equivalent.