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

propose alternative derivation mechanism #174

Open
yanns opened this issue Apr 16, 2020 · 3 comments
Open

propose alternative derivation mechanism #174

yanns opened this issue Apr 16, 2020 · 3 comments

Comments

@yanns
Copy link
Contributor

yanns commented Apr 16, 2020

in sphere-json & sphere-mongo we propose a generic derivation of typeclass instances based on scala macros & core generation.

I experimented using another derivation mechanism based on magnolia: https://github.com/sphereio/sphere-scala-libs/tree/derive_with_magnolia

I could use this new mechanism for sphere-mongo in our private backend.

Using magnolia instead of our own macros is a trade-of decisions:
(+) we can mutualize our efforts with other contributors to have a better derivation mechanism
(+) Magnolia advertises having good compilation time
(+) we could only rely on scala annotations and fix #131
(+) less code in this repository
(-) depending on magnolia
(-) less liberty

I personally think that this way is worth a try.
But I'd like to avoid a big-bang release where we move completely from our macros to magnolia.

My proposal:

  • phase 1: split out the current libraries to extract the current derivation
  • phase 2: add magnolia as a possible alternative. The 2 derivations mechanisms coexist. It means that we have to duplicate the tests. But this allow to quickly switch which mechanism we want to use
  • phase 3: depending on the results of phase 2, we could keep only magnolia

For the transition, we have 2 possibilities. For example if someone is using sphere-mongo:

  • 1st possibility: sphere-mongo can be a library depending on sphere-mongo-core & sphere-mongo-macro-derivation.
    • This change should be transparent for the user.
    • Users have to switch to sphere-mongo-core & sphere-mongo-magnolia to test in the phase 2.
  • 2nd possibility: we extract the derivation from sphere-mongo into sphere-mongo-macro-derivation.
    • We don't need any sphere-mongo-core.
    • Current users have to add this dependency if they rely on the derivation feature.
    • Users are then aware that we will propose sphere-mongo-magnolia in the phase 2 as an alternative to sphere-mongo-macro-derivation.
@agourlay
Copy link
Contributor

I personally think that this way is worth a try.

I share the same opinion, the pros are really attractive 👍

Regarding the transition, I think I prefer the second alternative, i.e without sphere-mongo-core. (Not a strong opinion).

yanns added a commit that referenced this issue Apr 29, 2020
yanns added a commit that referenced this issue Apr 29, 2020
Step 2 of #174

The tests are similar to the ones in sphere-mongo-derivation.
We have to make sure the keep those tests consistent in the future.
yanns added a commit that referenced this issue Apr 29, 2020
Step 2 of #174

The tests are similar to the ones in sphere-mongo-derivation.
We have to make sure the keep those tests consistent in the future.
@plokhotnyuk
Copy link

Json4s has a couple of not fixed vulnerabilities: https://github.com/json4s/json4s/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+denial

Please consider switching to a more safe parser (and AST, if it will be required) or help json4s maintainers to fix it.

@yanns
Copy link
Contributor Author

yanns commented May 25, 2020

Json4s has a couple of not fixed vulnerabilities: https://github.com/json4s/json4s/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+denial

Please consider switching to a more safe parser (and AST, if it will be required) or help json4s maintainers to fix it.

@plokhotnyuk thx for the feedback. This issue is about changing the derivation mechanism for the type classes and is not about the json parser.
I opened an issue for you about this: #203

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

No branches or pull requests

3 participants