Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store projections with proper type in in-mempory adapter #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OleMchls
Copy link
Contributor

Description

Currently, the in-memory adapter only stores the plain map data and thus behaves differently from the Postgres adapter.

Changes

  • Cast
  • [ ]

Concerns

🤷

Screenshots

😏

Currently the im-memory adapter only stores the plain map data and
thus behaves differently to the postgres adapter.
@pedroassumpcao
Copy link
Owner

@OleMchls first of all, thanks for the PR.

The fact that InMemory adapter is not fully similar to the PG is not a concern IMO, as the InMemory adapter works for playground or to be an option in case PG is not ideal or to explore things. The only major similarity should be that each projection is a list, and the items of list are equal in type and shape (same fields).

The fact that the InMemory adapter works the way it is now allows projections to be simple map keys (atoms or strings), for example, with no need to be module names, even that being another option as module names are atoms.

If we make the change you are proposing we will be adding some consistency with PG adapter regarding the projection name we will remove some flexibility from the InMemory adapter key naming.

If you run the test suite with the change you made you will see that 3 tests will fail because the tests are using projection keys that are not module names, and that key naming is enough for that specific case. So, it becomes a trade-off between flexibility and "consistency" but I see that adapter much more in need of being flexible to allow simple things to be done. Let me know your thoughts.

@OleMchls
Copy link
Contributor Author

OleMchls commented Mar 8, 2021

I understood/considered the various adapters, events, as well as projections to be interchangeable. I guess this stems from the name adapter. The actual implementation should be opaque to the consumer. I also noticed that when looking at some types:

@type persisted_event :: InMemory.Event.t() | Postgres.Event.t()

To me that is a pointer that it's not an actual adapter pattern. I understand your concern, and if you want the InMemory projection store to be the more flexible variant that's just fair. Your call 👍

I also did not catch the failing test, I assume the GitHub action workflow is not configured to run on PRs?

@pedroassumpcao
Copy link
Owner

Yes, I need to set up the GH actions for PRs as well.

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.

None yet

2 participants