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

SyncIO#>> parameter should be by-name #1947

Merged
merged 3 commits into from
May 9, 2021
Merged

SyncIO#>> parameter should be by-name #1947

merged 3 commits into from
May 9, 2021

Conversation

vasilmkd
Copy link
Member

@vasilmkd vasilmkd commented May 8, 2021

Unfortunately, this is a binary incompatible change...

@djspiewak
Copy link
Member

Ahhhh this is a concerning oversight. We should be able to avoid the binary breakage by overloading though. If we define a private[effect] def >>(...) eager version alongside the lazy version, it should satisfying the old binary linkage without polluting the public API.

@typelevel typelevel deleted a comment from djspiewak May 8, 2021
@vasilmkd
Copy link
Member Author

vasilmkd commented May 8, 2021

That's very smart. Recompiled code against the next version will pick up the new signature, right?

@djspiewak
Copy link
Member

That's very smart. Recompiled code against the next version will pick up the new signature, right?

Yep! It would only be old previously-linked code. This is kind of a general technique: when you want to break something, hide the old thing as package-private and, due to the granularity of Scala's access modifiers, you're basically getting the same ABI while still hiding the replaced entity from source resolution.

@vasilmkd
Copy link
Member Author

vasilmkd commented May 8, 2021

Awesome, thank you! I will push out the change shortly.

@vasilmkd vasilmkd marked this pull request as ready for review May 9, 2021 09:12
@vasilmkd vasilmkd merged commit 15c2735 into typelevel:series/3.x May 9, 2021
@vasilmkd vasilmkd deleted the synciobind branch May 9, 2021 09:13
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.

3 participants