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

Experimenting with by-name implicit parameters for generic derivation #1180

Merged
merged 6 commits into from Jun 24, 2019

Conversation

travisbrown
Copy link
Member

@travisbrown travisbrown commented Jun 23, 2019

This PR introduces a 2.13-only rewrite of circe-generic (as a new circe-generic-simple module, for now at least) that passes the same tests as circe-generic but completely removes DerivationMacros; the only macros are for the export mechanism and the @JsonCodec macro annotation. All of the shapeless.Lazy occurrences are gone—most have been replaced by by-name implicit parameters, but a few don't seem to be needed at all with 2.13.

In addition to having a less terrifying implementation, circe-generic-simple also provides much faster compilation (up to ~30% for some of my examples).

@travisbrown
Copy link
Member Author

Note that we'll need scalameta/scalameta#1880 for Scalafmt to work here (it seems like you can set runner.dialect = dotty as a workaround).

@codecov-io
Copy link

codecov-io commented Jun 23, 2019

Codecov Report

Merging #1180 into master will increase coverage by 0.31%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   85.58%   85.89%   +0.31%     
==========================================
  Files          89       89              
  Lines        2885     2885              
  Branches      115      119       +4     
==========================================
+ Hits         2469     2478       +9     
+ Misses        416      407       -9
Impacted Files Coverage Δ
...re/shared/src/main/scala/io/circe/KeyDecoder.scala 97.29% <0%> (-2.71%) ⬇️
...rc/main/scala/io/circe/numbers/BiggerDecimal.scala 90.05% <0%> (+2.92%) ⬆️
...re/shared/src/main/scala/io/circe/JsonNumber.scala 93.02% <0%> (+4.65%) ⬆️
...d/src/main/scala/io/circe/NonEmptySeqDecoder.scala 100% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b79152a...dcbd415. Read the comment docs.

@travisbrown travisbrown merged commit d8c8697 into master Jun 24, 2019
gen: LabelledGeneric.Aux[A, R],
codec: => ReprAsObjectCodec[R]
): DerivedAsObjectCodec[A] = new DerivedAsObjectCodec[A] {
final def apply(c: HCursor): Decoder.Result[A] = codec.apply(c) match {
Copy link

@johnynek johnynek Jun 24, 2019

Choose a reason for hiding this comment

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

Do you not need to cache codec in a lazy val to avoid executing it again and again for each call to apply?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I meant to go back and do a pass for runtime performance stuff like this after getting it correct. I've just opened #1183. Thanks for catching.

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

3 participants