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

Preserve binary compatibility for Factory #115

Merged
merged 4 commits into from Jul 26, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Jul 24, 2018

Replace the trait Factory by an abstract type upper-bounded by
CanBuildFrom, this way usages of CanBuildFrom can be replaced by Factory
without breaking binary compatibility.

This has the unfortunate consequence that all methods available on
CanBuildFrom are still available on Factory when compiling under old
Scala versions, but that seems like a price worth paying for BC.

Replace the trait Factory by an abstract type upper-bounded by
CanBuildFrom, this way usages of CanBuildFrom can be replaced by Factory
without breaking binary compatibility.

This has the unfortunate consequence that all methods available on
CanBuildFrom are still available on Factory when compiling under old
Scala versions, but that seems like a price worth paying for BC.
@smarter
Copy link
Member Author

smarter commented Jul 24, 2018

/cc @mpilquist @xuwei-k Could you check if that fixes the mima issues you got in https://github.com/scodec/scodec/pull/119/files ?

@smarter
Copy link
Member Author

smarter commented Jul 24, 2018

Also /cc king-of-binary-compatibility @sjrd to tell me if that makes sense :)

@sjrd
Copy link
Member

sjrd commented Jul 24, 2018

AFAICT, yes, it makes sense. A confirmation from MiMa would be good.

@julienrf
Copy link
Contributor

This has the unfortunate consequence that all methods available on
CanBuildFrom are still available on Factory when compiling under old
Scala versions, but that seems like a price worth paying for BC.

I think that’s fine because this code will be used to cross-compile with 2.13, and on 2.13 the CanBuildFrom API will raise compilation error if it used on a Factory instance.

@MasseGuillaume
Copy link
Member

I will add a follow-up PR to add mima as a unit test.

This is how you can call mima as a library: https://gist.github.com/MasseGuillaume/8b4ac3a7ec4e0d9fb9be0fb2e03b200c#file-cli-scala-L1

I will create two sbt projects then use BuildInfo to make their classpath available to the unit test.

@MasseGuillaume
Copy link
Member

@smarter Here you go: MasseGuillaume@945d1c8

You can cherry-pick this commit and add some tests.

@smarter
Copy link
Member Author

smarter commented Jul 26, 2018

@MasseGuillaume Thanks, done!

@smarter
Copy link
Member Author

smarter commented Jul 26, 2018

Maybe something needs to be done to run the added tests on the CI too?

@MasseGuillaume
Copy link
Member

MasseGuillaume@fecc6b5

@MasseGuillaume
Copy link
Member

@julienrf
Copy link
Contributor

Awesome guys!

@julienrf julienrf merged commit af0c28e into scala:master Jul 26, 2018
* @tparam A Type of elements (e.g. `Int`, `Boolean`, etc.)
* @tparam C Type of collection (e.g. `List[Int]`, `TreeMap[Int, String]`, etc.)
*/
type Factory[-A, +C] <: CanBuildFrom[Nothing, A, C] // Ideally, this would be an opaque type
Copy link
Contributor

Choose a reason for hiding this comment

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

@SethTisue
Copy link
Member

a new chapter in this story: #275

martijnhoekstra pushed a commit to martijnhoekstra/scala-collection-compat that referenced this pull request Nov 9, 2022
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

6 participants