Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
restrict traverse_ and friends to require Unit #4352
restrict traverse_ and friends to require Unit #4352
Changes from 3 commits
a9a0879
85e3d15
9f60b5c
b5ddca0
d35c6ea
b96d971
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is also causing binary compatibility issues, and in this case I don't see how this function can still exist in its current form (i.e. without said evidence) -- I guess it would have to move or something and that would for sure break binary compatibility? not sure if there is a fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we cannot change the signature of this method. What we can do is add a new ops class:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this one has to change then to
F.traverse_(fga)(G.void(_))
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That still allows people to call
.sequence
on non-unit structures -- I would suggest deprecating itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly, this is my feeling about this PR as a whole. It's obvious at the moment we don't even have existing test coverage to safely make this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we can't change signatures, what about adding new methods with an appropriate signature and deprecating the old ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall I did something similar – i.e. was tuning a method with a very subtle change in its signature while preserving its name: #3997. Not exactly the same though, but there are some similarities apparent.
Personally, I don't think it is a good idea, especially because the old methods do not have such bugs that would hinder their usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@armanbilge
Do you mean, there's no tests for
traverse_
norsequence_
or do you mean some specific test scenarios?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satorg see #4352 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes will also cause compatibility problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed the correct answer here would be to leave the prior implicit params alone and just add mine at the end of arglist. the mima check is still complaining in 2.12 and I'm not sure how to resolve it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately we cannot change these signatures at all: no changes, no additions, no removals. It's not specific to Scala 2.12, that one probably just happened to fail first.
See #4324 (comment) for a possible strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.