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

Redesign the configurability of the derived serializers #36

Closed
wants to merge 1 commit into from

Conversation

julienrf
Copy link
Owner

@julienrf julienrf commented Jul 3, 2016

This PR changes the way the derived marshallers can be configured.

Before, the PR, a DerivedReads[A] had a method def reads(typeTagReads: TypeTagReads): Reads[A] that was using the typeTagReads parameter to customize the resulting Reads[A].

With this PR, the configuration is retrieved from implicit parameters, during the derivation process. This makes it possible to use type-directed mechanisms for configuring field names, as in #33. With the previous design, this was harder to achieve with the same efficiency.

However, I still think that the design proposed by this PR has some drawbacks:

  • The configuration is driven by implicits, meaning that the same line of code will behave differently, according to what is in the implicit scope ;
  • We have to take care to use unique names for values that have to be explicitly imported ;
  • If you forget to import your custom configuration nothing will help you to notice it (no compile error).

The first point is the one that bothers me the most. The last point could be solved by not having a default configuration: users would be forced to import one configuration for their code to compile, and, if two configurations were imported, that would result in an ambiguous implicit values error.

@julienrf
Copy link
Owner Author

julienrf commented Jul 9, 2016

@gcpagano Any thougth on this?

@gcpagano
Copy link
Contributor

Looks good, it's definitely a viable alternative.
I agree with the drawbacks you have highlighted, I think I would prefer not to have a default configuration and force the user to import one.

I slightly prefer the initial design of having explicit parameters to configure the derivation process, I think that way is cleaner and less error prone.

Unfortunately I haven't had time to finish porting my PR on class members adapter to use the explicit config. I'll try to find some time in the coming week

@julienrf julienrf added this to the v4 milestone Sep 13, 2016
@julienrf julienrf removed this from the v4 milestone Jul 4, 2017
@julienrf
Copy link
Owner Author

I’m closing this one since it is not active anymore.

@julienrf julienrf closed this May 13, 2021
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