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

Stdlib: Add function composition combinator #12770

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Conversation

laelath
Copy link
Contributor

@laelath laelath commented Nov 22, 2023

Add Fun.compose and Stdlib.(@.) for function composition. Useful when wanting to apply multiple functions when wanting to map over a list as List.map (f @. g) l exposes the high-level logic in a clearer manner than List.map (fun x -> f (g x)) l. The choice of @. was made because Haskell uses . for function composition and there is probably a clearer choice that could be made for OCaml.

@laelath laelath changed the title Add function composition combinators Add function composition combinators to stdlib Nov 22, 2023
@laelath laelath changed the title Add function composition combinators to stdlib Stdlib: Add function composition combinator Nov 22, 2023
@nojb
Copy link
Contributor

nojb commented Nov 22, 2023

My opinion based on a quick look:

  • In favour of adding Fun.compose,
  • Not really in favour of adding the infix operator: 1) I think the general direction is not to add anything else to Stdlib itself to avoid polluting the global namespace, and 2) infix operators tend to harm code readability, and I'm not sure we want to encourage that.

@nojb nojb added the stdlib label Nov 22, 2023
@laelath
Copy link
Contributor Author

laelath commented Nov 22, 2023

While overuse of infix operators can certainly harm code readability, I think that more sparing/deliberate usages of them can make things clearer. For the purposes of this PR consider f @. g @. h vs compose f (compose g h). Maybe the infix operators could be added without polluting the global namespace by defining a Fun.Operators sub module that can be opened to access?

@Octachron
Copy link
Member

Octachron commented Nov 23, 2023

There is also the issue that the choice of @. is non-standard. A [quick look on sherlocode](('[a-z] -> '[a-z]) -> ('[a-z] -> '[a-z]) -> '[a-z] -> '[a-z]) yields the following operator name for compose.

  • |., -|
  • <<<, <<
  • $, %
  • @*
  • ++

In other words, the name of the operator is controversial, and it is better to split this discussion on the operator from the simple compose function.

I am personally in favor of settling a name for the composition operator, but this is better done in a large discussion rather than in a PR.

@dbuenzli
Copy link
Contributor

This has already been discussed to death here.

@xvw
Copy link
Contributor

xvw commented Nov 23, 2023

In Preface we use f % g for f . g (and f %> g for g . f, wich is never really used) and we have some case where it is useful and probably more readable than (fun x -> f (g x)). And since we have a module Arrow for type ('a, 'b) t = 'a -> 'b) we can also use >>> and <<< but we take the habit to use %.

@laelath
Copy link
Contributor Author

laelath commented Nov 25, 2023

I removed the infix operator for now

Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

I am in favour of this addition. As it touches the standard library, a second approval from a core dev is needed.

@nojb
Copy link
Contributor

nojb commented Nov 28, 2023

Friendly ping. This is a simple PR to review; we still need a second reviewer; volunteers welcome!

@dbuenzli
Copy link
Contributor

I looked at it today. It's ok with me if someone wants to ack on my behalf.

Copy link
Member

@yallop yallop left a comment

Choose a reason for hiding this comment

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

Approved (on behalf of @dbuenzli, but I'm happy with it, too)

@nojb
Copy link
Contributor

nojb commented Nov 28, 2023

Thanks for the prompt review! I took the liberty of amending the Changes file accordingly.

@nojb nojb added the merge-me label Nov 28, 2023
@nojb nojb merged commit 3963eac into ocaml:trunk Nov 29, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants