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

Enable multiple function composition in prefix form #33568

Merged
merged 16 commits into from
Oct 16, 2019
Merged

Enable multiple function composition in prefix form #33568

merged 16 commits into from
Oct 16, 2019

Conversation

JeffFessler
Copy link
Contributor

Document the prefix form and extend the prefix form to support composition of 3 or more functions.
If there is favorable sentiment for this then I can also add tests.
There was discussion in 2016 about related issues but I don't see anything more recent
#17184

Document the prefix form and extend the prefix form to support composition of 3 or more functions.
If there is favorable sentiment for this then I can also add tests.
There was discussion in 2016 about related issues but I don't see anything more recent
#17184
base/operators.jl Outdated Show resolved Hide resolved
Thanks for the feedback.  Done.
Also added a single doctest that should suffice for codecov.
base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
I also removed "for functions" because composition can work more generally it seems.
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

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

Since this is a new feature, I guess we will need to announce it in a one-liner in NEWS?

base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
base/operators.jl Outdated Show resolved Hide resolved
@StefanKarpinski
Copy link
Member

Nice addition! Makes perfect sense that composition should work for arbitrary args.

JeffFessler and others added 2 commits October 15, 2019 15:10
Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
I was trying to minimize number of lines changed :)

Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
@StefanKarpinski StefanKarpinski added needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Oct 15, 2019
JeffFessler and others added 3 commits October 15, 2019 15:25
Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
base/operators.jl Outdated Show resolved Hide resolved
Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
base/operators.jl Outdated Show resolved Hide resolved
I was avoiding the term "function" because an earlier version had unnecessary `Function` typing but I agree that the description is more clear this way.

Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
@StefanKarpinski
Copy link
Member

Looking good. Next round of improvements:

  1. A couple of simple tests (although there's already the doc test)
  2. Add a !!!compat "Julia 1.4" annotation in the docstring indicating that this is a new feature
  3. Add an entry in NEWS linking to this PR and briefly describing the feature.

spurious `true` left from older version of doctest
@JeffFessler
Copy link
Contributor Author

I've done the "next round of improvements" list. Just made some last tweaks...

@StefanKarpinski StefanKarpinski removed needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change needs tests Unit tests are required for this change labels Oct 16, 2019
@StefanKarpinski StefanKarpinski merged commit c8313c6 into JuliaLang:master Oct 16, 2019
tkf added a commit to tkf/Compat.jl that referenced this pull request Oct 16, 2019
tkf added a commit to tkf/Compat.jl that referenced this pull request Oct 16, 2019
@dkarrasch
Copy link
Member

A bit off-topic: @StefanKarpinski I saw you dismissed my earlier review, which was referring to a much earlier state of the PR. Should I have actively changed my review assessment as to not confuse people? Obviously, it was not meant to "request changes" indefinitely. Can I take back a "request changes" without necessarily "approving" the PR?

@fredrikekre
Copy link
Member

fredrikekre commented Oct 16, 2019

This PR failed CI due to trailing whitespace. Edit: #33574

@StefanKarpinski
Copy link
Member

Hmm. I'm not sure. I think you can mark individual comments as resolved but I'm not sure how you mark a review as resolved other than what I did — i.e. dismissing it, which felt a bit, uh, dismissive, but seemed like the only thing to do in order to let the PR be mergeable. Maybe leaving a "comment" review puts it into a neither approved nor denied state?

@StefanKarpinski
Copy link
Member

It's really annoying that GitHub will show you the deletion of trailing whitespace in a diff but won't show you the insertion of trailing whitespace, so it's impossible to tell by reviewing a diff that it introduces trailing whitespace. Sorry I missed the whitespace failure in CI. I'm so used to a lone failing CI test for simple changes like being inconsequential that I just ignored it out of habit.

@JeffFessler
Copy link
Contributor Author

Thanks everyone - one line of new code and two dozen lines of comments and tests :)

@JeffFessler JeffFessler deleted the patch-2 branch October 16, 2019 19:40
@StefanKarpinski
Copy link
Member

That's the way it goes 😁

martinholters pushed a commit to JuliaLang/Compat.jl that referenced this pull request Oct 20, 2019
@jw3126
Copy link
Contributor

jw3126 commented Jan 2, 2020

I think we should also have

() = identity
(f) = f

@StefanKarpinski
Copy link
Member

Seems like a good addition. Care to make a PR, @jw3126?

@jw3126
Copy link
Contributor

jw3126 commented Jan 3, 2020

Sure thing!

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.

6 participants