Skip to content

Tweaks to ChainingOps #7036

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.

Sign up for GitHub

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

Jump to bottom
Merged
SethTisue merged 4 commits into from
Aug 10, 2018
Merged

Tweaks to ChainingOps #7036

SethTisue merged 4 commits into from
Aug 10, 2018

Conversation

@dwijnand
Copy link
Member

@dwijnand dwijnand commented Aug 10, 2018

No description provided.

@dwijnand
Make ChainingOps#tap return A
f38d1ce 
.. so it returns a sane type & works with AnyVal types.
@dwijnand
Make ChainingOps keep its self private
f1709cf 
.. to avoid giving Any type a self method.
@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 10, 2018
@dwijnand dwijnand mentioned this pull request Aug 10, 2018
Merged
NthPortal
NthPortal approved these changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

LGTM

xuwei-k
xuwei-k approved these changes Aug 10, 2018
View reviewed changes
Copy link
Contributor

@xuwei-k xuwei-k left a comment

Choose a reason for hiding this comment

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

👍

@dwijnand
Copy link
Member Author

dwijnand commented Aug 10, 2018

squeezes in at the back of the queue, just as the doors are closing

@dwijnand dwijnand modified the milestones: 2.13.0-RC1, 2.13.0-M5 Aug 10, 2018
@dwijnand
Copy link
Member Author

dwijnand commented Aug 10, 2018

@lrytz you suggested returning self.type [1], given @xuwei-k's findings [2][3] are you happy to change it to A?

@dwijnand dwijnand requested a review from lrytz August 10, 2018 09:34
@lrytz
Copy link
Member

lrytz commented Aug 10, 2018

Sounds good. Could you add tests to this PR?

@dwijnand
Copy link
Member Author

dwijnand commented Aug 10, 2018

@lrytz Temporarily reverting the self.type result type doesn't make the test fail, I'm not sure why. But the test should always pass, so I kept it.

Good to go, for you?

@eed3si9n
Copy link
Member

eed3si9n commented Aug 10, 2018

Thanks for the cleanup.

@SethTisue SethTisue merged commit d746fbf into scala:2.13.x Aug 10, 2018
@dwijnand dwijnand deleted the chaining-ops-tweaks branch August 10, 2018 19:50
bigwheel added a commit to bigwheel/tapandpipe that referenced this pull request Aug 21, 2018
@bigwheel
Add tap and pipe code by scala/scala PR
95084c6 
scala/scala#7007
scala/scala#7036
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xuwei-k xuwei-k xuwei-k approved these changes

@lrytz lrytz Awaiting requested review from lrytz

+1 more reviewer

@NthPortal NthPortal NthPortal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.13.0-M5

Development

Successfully merging this pull request may close these issues.

7 participants

@dwijnand @lrytz @eed3si9n @xuwei-k @NthPortal @SethTisue @scala-jenkins