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

make Tagged a universal trait so tagging value classes works #801

Merged
merged 3 commits into from
Mar 15, 2020

Conversation

kwark
Copy link
Contributor

@kwark kwark commented Dec 31, 2017

This fixes issue #644

I also ran mima and it did not find any binary incompatible changes:

[IJ]> mimaReportBinaryIssues
[info] Resolving com.chuusai#shapeless_2.12;2.3.2 ...
[info] Resolving org.scala-lang#scala-library;2.12.0 ...
[info] Resolving org.typelevel#macro-compat_2.12;1.1.1 ...
[info] Resolving com.chuusai#shapeless_sjs0.6_2.12;2.3.2 ...
[info] Resolving org.scala-lang#scala-library;2.12.0 ...
[info] Resolving org.scala-js#scalajs-library_2.12;0.6.13 ...
[info] Resolving org.typelevel#macro-compat_2.12;1.1.1 ...
[info] Compiling 2 Scala sources to /Users/peter/dev/shapeless/core/jvm/target/scala-2.12/classes...
[info] Compiling 2 Scala sources to /Users/peter/dev/shapeless/core/js/target/scala-2.12/classes...
[info] Compiling 13 Scala sources to /Users/peter/dev/shapeless/core/jvm/target/scala-2.12/classes...
[info] Compiling 13 Scala sources to /Users/peter/dev/shapeless/core/js/target/scala-2.12/classes...
[info] Compiling 85 Scala sources to /Users/peter/dev/shapeless/core/jvm/target/scala-2.12/classes...
[info] Compiling 85 Scala sources to /Users/peter/dev/shapeless/core/js/target/scala-2.12/classes...
[info] coreJVM: found 0 potential binary incompatibilities while checking against com.chuusai:shapeless_2.12:2.3.2  (filtered 6)
[info] root: previous-artifact not set, not analyzing binary compatibility
[info] coreJS: found 0 potential binary incompatibilities while checking against com.chuusai:shapeless_sjs0.6_2.12:2.3.2  (filtered 6)
[success] Total time: 49 s, completed Dec 31, 2017 9:31:01 AM

@codecov-io
Copy link

codecov-io commented Dec 31, 2017

Codecov Report

Merging #801 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #801   +/-   ##
=======================================
  Coverage   87.99%   87.99%           
=======================================
  Files          64       64           
  Lines        1499     1499           
  Branches        5        5           
=======================================
  Hits         1319     1319           
  Misses        180      180
Impacted Files Coverage Δ
core/src/main/scala/shapeless/typeoperators.scala 0% <ø> (ø) ⬆️

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 6ce7329...9e1a135. Read the comment docs.

@milessabin
Copy link
Owner

Thanks for this ... I'm literally about to push the button on releasing 2.3.3, so this has missed the boat unfortunately.

This looks good, but we've made several attempts to improve things in this area before and they've all turned out to have downsides of one sort or another. I have a vague feeling that we've actually tried exactly this change previously ... would you mind going through the related PRs and issues and see if you can work out where things stand?

@kwark
Copy link
Contributor Author

kwark commented Jan 1, 2018

I have been going through some related PRs:

I don't find any evidence that this specific change has been tried previously.
I do acknowledge that this kind of change might inadvertently impact other areas of the shapeless codebase.

@milessabin I'll be happy to check other PRs/issues. Do you have any specific in mind?

@joroKr21 joroKr21 added this to the shapeless-2.4.0 milestone Jan 12, 2020
@joroKr21
Copy link
Collaborator

Needs rebase. Is there any reason not to do this? Looks like a no-brainer to me.

@milessabin
Copy link
Owner

Is there any reason not to do this? Looks like a no-brainer to me.

Issues with 2.10 maybe?

@joroKr21
Copy link
Collaborator

We dropped 2.10 🎉

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

4 participants