Skip to content

Typeclasses requiring only read access, and private constructors #222

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

Closed
jatcwang opened this issue Mar 30, 2020 · 8 comments
Closed

Typeclasses requiring only read access, and private constructors #222

jatcwang opened this issue Mar 30, 2020 · 8 comments

Comments

@jatcwang
Copy link
Contributor

Many typeclasses require only read access to fields (e.g.Show, diffx's Diff, JSON Encoders), but due to the existence of CaseClass#construct, magnolia derivation will fail if the primary constructor is not accessible.

Proposal:

  • When a library author writes def combine[T](ctx: CaseClass[Typeclass, T]): Derived[MyTypeClass[T]], allow them to use def combine[T](ctx: ReadOnlyCaseClass[Typeclass, T]): Derived[MyTypeClass[T]] instead, where ReadOnlyCaseClass will not have the construct methods.
  • Magnolia.gen will detect that ReadOnlyCaseClass is used and not try to resolve constructors

This applies to abstract as well, as currently sealed abstract case class is quite a common pattern in < 2.12 due to scala/bug#7884 (Should be fixed in 2.13.2?)

WDYT?

@joroKr21
Copy link
Contributor

I think it makes sense, but I'm not sure if the encoding you are proposing has the best UX.

@propensive
Copy link
Collaborator

I'm not sure either, but I can't think of anything that's obviously better. The change shouldn't be too difficult, with this approach.

@jatcwang
Copy link
Contributor Author

Adding a super class to CaseClass should be binary compatible (even if I move some methods like parameters to the parent ReadOnlyCaseClass).

@joroKr21
Copy link
Contributor

We haven't promised binary compatibility yet anyway.

@jatcwang
Copy link
Contributor Author

As a downstream user I'd prefer to not deal with any MethodNotFoundExceptions ;) It will probably be ok.... 🤞

@propensive
Copy link
Collaborator

I did bump the version number from 0.12.x to 0.13.x in the last (as-yet unannounced) release on the basis that it included a small API change, but in general I haven't been careful about it...

@joroKr21
Copy link
Contributor

Yeah if we really want to make sure, then we should cut a 1.0 release and add MiMa.

@propensive
Copy link
Collaborator

I'd like to leave that a while. I'm a bit reluctant to give a binary compatibility guarantee while I at least expect to change a few things... my priority right now is to make it easier to publish both Magnolia and ~20 other libraries...

jatcwang added a commit to jatcwang/magnolia that referenced this issue Apr 4, 2020
jatcwang added a commit to jatcwang/magnolia that referenced this issue Apr 4, 2020
propensive pushed a commit that referenced this issue Apr 7, 2020
#224)

* Support typeclass that only needs to read fields (#222)

* add benchmark module

* Update scaladoc

* Quasiquotes: Change to use static package from the macro context when referring to magnolia and scala package name space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants