docs: fix bug/typo in tutorial part 2

[gregbrowndev]

The code in the tutorial shows how to write a command that triggers the event using trigger_event() where the mutate method is implemented on the event. However, there is a bug in the document as it should return the aggregate instance. Otherwise, it is not possible to complete the exercise later on in the tutorial using this explicit approach.

For example, here's my solution to the exercise:

from eventsourcing.domain import Aggregate, event

class Todos(Aggregate):
    class Started(Aggregate.Created):
        name: str

    @event(Started)
    def __init__(self, name: str):
        self.name = name
        self.items = []

    class ItemAdded(Aggregate.Event):
        item: str

        def mutate(self, aggregate: "Todos"):
            aggregate.items.append(self.item)
            return aggregate  # NOTE: the aggregate must be returned by `mutate`

    def add_item(self, item: str):
        self.trigger_event(self.ItemAdded, item=item)

without this, the assertion in the code block below fails because e.mutate for the first TrickAdded event returns None and then the second TrickAdded event throws AttributeError: 'NoneType' object has no attribute 'items'.

    # Reconstruct aggregate from events.
    copy = None
    for e in events:
        copy = e.mutate(copy)
    assert copy == todos1

Also, looking at the function in CanMutateAggregate::mutate shows:

    def mutate(self, aggregate: Optional[TAggregate]) -> Optional[TAggregate]:
        """
        Changes the state of the aggregate
        according to domain event attributes.
        """
        assert aggregate is not None
        ...

The return value is never None. Would it be clearer to make the signature non-optional?

Alternatively, would the documented usage of e.mutate be better expressed where the instance copy is mutated, or does this not work with the other styles? i.e.

    # Reconstruct aggregate from events.
    copy = None
    for e in events:
       e.mutate(copy)  # no assignment to copy
    assert copy == todos1

[coveralls]

Pull Request Test Coverage Report for Build 2319109780

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report. To ensure accuracy in future PRs, please see these guidelines. A quick fix for this PR: rebase it; your next report should be accurate.

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 2297238857:	0.0%
Covered Lines:	4818
Relevant Lines:	4818

---

💛 - Coveralls

[johnbywater]

Hey @gregbrowndev! Thanks for spotting this and creating the PR! I appreciate your contribution.

Some further thoughts from me:

1. By the way, when implementing the mutate() method, it's also important to call super().mutate(aggregate). That is, it's important both to call "super" and to return a value.

2. For this reason, the better thing in cases like this is to call the apply() method, which is a "helper method" called by the base class mutate method, which doesn't normally need to call "super" and isn't expected to return a value.

3. This style, of defining methods on the event, is a little bit odd, in that the normal intuition for methods is that the argument is used to set attributes on the object being called, but in this case the situation is reversed with the attributes of the object being called (the event) being used to set attributes on the argument passed into the method (the aggregate). And although this is possible, and described in the module docs, it probably isn't something that should be included in this tutorial. So I will merge this PR but then probably adjust the example to provide for the case where work needs to be done by the command before the event is triggered by decorating a "private" method with the @event decorator.

4. Regarding the optional typing on the return value: this is to support the possibility of returning None which is what "discarded" events do. That results in the projection being the same as if there weren't any events, and after storing such an event, the repository will raise a "not found" exception. This isn't actually implemented in v9. But the optional typing provides for this, and I should at least add an example showing how this case be done. We've been discussing this in the Slack workspace, which you are very welcome to join, if you would like to.

Thanks again for spotting and fixing this issue.

[johnbywater]

I've resolved things like this (version 9.2.13 just released): https://eventsourcing.readthedocs.io/en/v9.2.13/topics/tutorial/part2.html#explicit-style

Thanks again for raising this issue @gregbrowndev.

If you have any other fixes, comments, suggestions, or questions, please don't hesitate!