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.

Sign up for GitHub

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

Jump to bottom

Fixes options and lists serialization in proto #342

Merged
merged 2 commits into from Jul 17, 2018
Merged

Fixes options and lists serialization in proto #342

merged 2 commits into from Jul 17, 2018

Conversation

fedefernandez
Copy link
Contributor

Fixes #285

Thanks to @btlines that pointed me in the right direction (see btlines/pbdirect#21)

To serialise Option correctly you need to import cats.implicits._ or more specifically import cats.instances.option._. (I suppose you have this import clause present in your tests somehow).

The reason is that pbdirect knows how to serialise a functor but for this to work it needs an evidence that Option is a Functor and this is provided by importing cats implicits.

@fedefernandez fedefernandez self-assigned this Jul 17, 2018
@fedefernandez fedefernandez requested a review from a team July 17, 2018 07:21
juanpedromoreno
juanpedromoreno approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Thanks @fedefernandez !

Does it make sense to do a patch release with this fix?

pepegar
pepegar approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@pepegar pepegar left a comment

Choose a reason for hiding this comment

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

👏 👏

@fedefernandez
Copy link
Contributor Author

Sure, thanks

@L-Lavigne L-Lavigne self-requested a review July 17, 2018 07:49
L-Lavigne
L-Lavigne approved these changes Jul 17, 2018
View reviewed changes
Copy link
Contributor

@L-Lavigne L-Lavigne left a comment

Choose a reason for hiding this comment

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

👏

@fedefernandez fedefernandez merged commit 169d3ba into master Jul 17, 2018
@fedefernandez fedefernandez deleted the ff-285-options-list-proto branch July 17, 2018 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pepegar pepegar pepegar approved these changes

@juanpedromoreno juanpedromoreno juanpedromoreno approved these changes

@JesusMtnez JesusMtnez JesusMtnez approved these changes

@L-Lavigne L-Lavigne L-Lavigne approved these changes

Assignees

@fedefernandez fedefernandez

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fedefernandez @pepegar @juanpedromoreno @JesusMtnez @L-Lavigne