Enable multiple function composition in prefix form #33568
Conversation
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
Thanks for the feedback. Done. Also added a single doctest that should suffice for codecov.
I also removed "for functions" because composition can work more generally it seems.
|
Nice addition! Makes perfect sense that composition should work for arbitrary args. |
Co-Authored-By: Stefan Karpinski <[email protected]>
I was trying to minimize number of lines changed :) Co-Authored-By: Stefan Karpinski <[email protected]>
StefanKarpinski
added
needs compat annotation
Co-Authored-By: Stefan Karpinski <[email protected]>
Co-Authored-By: Stefan Karpinski <[email protected]>
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 <[email protected]>
|
Looking good. Next round of improvements:
|
spurious `true` left from older version of doctest
|
I've done the "next round of improvements" list. Just made some last tweaks... |
StefanKarpinski
removed
needs compat annotation
|
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? |
|
This PR failed CI due to trailing whitespace. Edit: #33574 |
|
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? |
|
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. |
|
Thanks everyone - one line of new code and two dozen lines of comments and tests :) |
|
That's the way it goes 😁 |
|
I think we should also have ∘() = identity
∘(f) = f |
|
Seems like a good addition. Care to make a PR, @jw3126? |
|
Sure thing! |
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