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

Remove unnecessary Result type, replacing with isomorphic Either #183

Merged
1 commit merged into from
Nov 16, 2021

Conversation

georgefst
Copy link
Contributor

This is effectively a follow-up to https://github.com/hackworthltd/vonnegut/pull/775. It wasn't noticed back then that somehow we had two workarounds for Either serialisation - Data.Sum and App.Result.

There is a slight change to our encodings, since we were using aeson's defaultTaggedObject sum encoding, whereas Either's instance uses the more concise ObjectWithSingleField. See the changes to test fixtures.

@dhess
Copy link
Member

dhess commented Oct 21, 2021

This seems like a no-brainer.

@dhess
Copy link
Member

dhess commented Oct 21, 2021

(Although perhaps wait to merge this until after #180 is merged?)

@georgefst
Copy link
Contributor Author

(Although perhaps wait to merge this until after #180 is merged?)

Yep, there will be a minor conflict, and it's probably marginally easier to rebase this one.

@dhess
Copy link
Member

dhess commented Nov 16, 2021

OK, let's merge this one now?

This is effectively a follow-up to hackworthltd/vonnegut#775. It wasn't noticed back then that somehow we had two workarounds for `Either` serialisation - `Data.Sum` *and* `App.Result`.

There is a slight change to our encodings, since we were using `aeson`'s `defaultTaggedObject` sum encoding, whereas `Either`'s instance uses the more concise `ObjectWithSingleField`. See the changes to test fixtures.
@georgefst
Copy link
Contributor Author

bors merge

@ghost
Copy link

ghost commented Nov 16, 2021

@ghost ghost merged commit 6cba710 into main Nov 16, 2021
@ghost ghost deleted the georgefst/result-to-either branch November 16, 2021 14:16
This pull request was closed.
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