[Platform] Streamline message bag integration #1020
Conversation
- Add and prepand behave same now (do not clone the object, keep the state / avoid that `prepand` also clone the object, because only with* methods should do so) - Add and prepand both return the current object for chain-calls - Add a ID for the message bag - Renew the ID in case of cloning the message bag - Merge do NOT clone the object anymore, because it just merge messages (clone can be done manually, if needed) - Remove the `with` method, because this was used just once (keep the focus to explicit clone and add, if needed) - Add `IteratorAggregate` interface for easy access the current messages Fix symfony#962 Related symfony#1018
lochmueller
requested review from
Nyholm,
OskarStark and
chr-hertel
as code owners
carsonbot
added
Bug
|
Thanks for that - ID & iterator make def sense and generally I'm in favor of what's going with the cloning here - i only struggle a bit with having the fluent interface for |
…arStark) This PR was merged into the main branch. Discussion ---------- [Platform] Add `IteratorAggregate` to `MessageBag` | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | no | Issues | -- | License | MIT Allow iterating over MessageBag using foreach by implementing IteratorAggregate interface with ArrayIterator. Extracted from * #1020 cc `@lochmueller` Commits ------- da29d5c [Platform] Add IteratorAggregate to MessageBag
|
I extracted the array iterator to a separate PR, so please rebase, thanks. Can you extract the ID to a dedicated PR too? |
|
hey @chr-hertel thanks for the first steps. I moved the ID part to a separated PR: #1027 For the other stuff... I suggest that only "with*" and "without*" methods should clone the message Bag (like for immutable objects). "Add" and "prepend" are a little bit strange, because the one clone the object and the other not. I would close this PR and create a new one, if you confirm, that the "prepend" shouldn't clone the object? Next question.... "add & prepend" are not really a good "naming combination". I thought about "append & prepend"? Should I change the Naming also? Regards, |
|
Don't really have an issue with the naming tbh. The prepend was originally introduced for bringing system prompts in on messagebags without it. And cloning makes sense in that case. Let's have a look at the use cases for the decision. Do we have others in mind? Do you use prepend in your code? |
4 participants
What do you think?
prepandalso clone the object, because only with* methods should do so)withmethod, because this was used just once (keep the focus to explicit clone and add, if needed)IteratorAggregateinterface for easy access the current messagesFix #962
Related #1018