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

Poison to Jason replacement & Postgrex update changes #144

Merged
merged 4 commits into from
Jan 21, 2019

Conversation

vasspilka
Copy link
Contributor

No description provided.

@vasspilka vasspilka mentioned this pull request Nov 13, 2018
@vasspilka vasspilka changed the title replace poison with jason [WIP] replace poison with jason Nov 13, 2018
@slashdotdash
Copy link
Member

slashdotdash commented Nov 14, 2018

Jason requires you to manually derive the Jason.Encoder protocol for all user defined structs (by adding @derive Jason.Encoder before defstruct in the module). Migrating to Jason will require all EventStore users to add the protocol to every one of their own domain event modules.

We need to ensure this is included in the CHANGELOG as a breaking change with a warning.

@vasspilka vasspilka changed the title [WIP] replace poison with jason Poison to Jason replacement & Postgrex update changes Nov 16, 2018
test/support/json_serializer.ex Outdated Show resolved Hide resolved
@danhawkins
Copy link

@vasspilka are you still activly working on this, if not I can try to pick it up


case type do
[] -> Jason.decode!(binary)
type -> struct(type, Jason.decode!(binary, keys: :atoms))

Choose a reason for hiding this comment

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

It should be safer to use :atoms! to only use existing atoms when loading data. To make sure all the atoms from the struct are always loaded, you could first make sure that the struct itself is loaded with something like struct(struct(type), Jason.decode!(binary, keys: :atoms!)).

Choose a reason for hiding this comment

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

Oh, the additional struct call is already there. So only the :atoms to :atoms! change would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it

@vasspilka
Copy link
Contributor Author

@danhawkins Hey, I am having trouble figuring out how to make the tests pass. If you manage to figure out why they don't we can fix and merge this.

@vasspilka
Copy link
Contributor Author

vasspilka commented Dec 24, 2018

I am not "actively" working on it, but I can try to have a look again these days. Also you started using eventstore on Quiqup?

@vasspilka vasspilka force-pushed the master branch 4 times, most recently from 50dd92b to a95a631 Compare January 18, 2019 16:42
@slashdotdash slashdotdash merged commit a2c3e6e into commanded:master Jan 21, 2019
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.

5 participants