Skip to content

Added read events repository #19

Open
RichyBaldwin wants to merge 2 commits intoBullOak:masterfrom
RichyBaldwin:read-events
Open

Added read events repository #19
RichyBaldwin wants to merge 2 commits intoBullOak:masterfrom
RichyBaldwin:read-events

Conversation

@RichyBaldwin
Copy link

  • Added read events repository interface and implementation
  • Fixed typo

Copy link
Contributor

@iblazhko iblazhko left a comment

Choose a reason for hiding this comment

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

LGTM, but this change brings the question about overall library design.

Now that we are exposing interface to read events "as-is", I may also need to run custom appliers on that set, in the same way BullOak does.

Basically, what I am trying to say is that it may make sense to consider different interface for the event store where a user of the library can supply an explicit appliers / projection implementation, to better separate events reading from running appliers.

Here is a snippet for the possible interface definition

type EventStream<'Event> =
    { StreamId : EventStreamId.T
      Version : StreamVersion.T
      Events: 'Event seq }

type EventStreamProjection<'Event, 'State> =
    { New : EventStreamId.T -> 'State
      Apply : 'State -> 'Event -> 'State }

type EventStreamSession<'Event, 'State> =
    { GetStream : unit -> Async<EventStream<'Event>>
      Fold: EventStreamProjection<'Event,'State> -> Async<'State>
      AppendEvents: 'Event seq -> unit
      Save: unit -> Async<unit> }

type EventStore<'Event, 'State> =
    { Get: EventStreamId.T -> Async<EventStreamSession<'Event,'State>>
      Delete: EventStreamId.T -> Async<unit>
      Contains: EventStreamId.T -> Async<bool> }

(note the Fold in EventStreamSession that runs user-supplied function)

Anyway, that's probably for @skleanthous to ponder about, I am not really sure if this change will actually be useful.

This PR LGTM as it is.

And I try to save the new events in the stream through their interface
When I read all the events back from the stream
Then the load process should succeed
And I should see all the events
Copy link
Contributor

Choose a reason for hiding this comment

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

"the load process should succeed" looks redundant given we are checking for "I should see all the events"

Tests below do not check for "the load process should succeed" and I think this is how it should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants