[Proposal] Add `then/3` that allows for conditionally calling function

[Caio Oliveira]

I opened a PR (https://github.com/elixir-lang/elixir/pull/13758) but Valim asked me to post here first to get feedback for the community. The gist is turning code like this:

x
|> then(fn val ->
  if pred(&1) do
    f(val)
  else
    val
  end)

Into this:

x |> then(&f/1, if: &pred/1)

[Greg Vaughn]

You can already capture the `if` and do it as a one-liner

x |> then(&if(pred(&1), do: f(&1), else: &1))

so you don't gain much yet add more complexity. Additionally, your suggestion only implies what happens if `pred` is falsy while mine is clear.

-Greg Vaughn

[Caio Oliveira]

True, although personally I find the one-line `if` kind of confusing, especially when I was newer to the language.

> Additionally, your suggestion only implies what happens if `pred` is falsy while mine is clear.

This almost convinced me, to be honest! But on the flip side this makes it possible for you to forget to add a `else` clause, and just end up with a `nil` that is potentially hard to find where it came from.

Jose replied this in the PR (including here to centralize the discussion and not spam the PR and his email there):

> I personally would prefer to write the original code or use no pipeline at all. I don’t think the gain in conciseness justifies the loss in readability.

I'd say I agree with it 99% of the time, but there's this 1% that makes me miss Clojure's `cond->`. The concrete example that made me write this PR was this: I'm writing a small internal library to build requests for a third party service. There are some options that, if included, requires me to add headers to a request. The code looks something like this:

```elixir

def request_entity(opts) do

  modified_since = Keyword.get(opts, :modified_since)

  entity_id = Keyword.get(opts, :entity_id)

  Req.new(url: "example.com")

  |> add_x()

  |> authorize()

  |> add_body()

  |> then(&if(modified_since, do: Req.put_header(&1, "If-Modified-Since", modified_since), else: &1))
  |> then(&if(entity_id, do: Req.put_header(&1, "X-Entity-Id", entity_id), else: &1))

  |> Req.request()

end

```

And I'd much rather write something like this instead:

```elixir

def request_entity(opts) do

  modified_since = Keyword.get(opts, :modified_since)

  entity_id = Keyword.get(opts, :entity_id)

  Req.new(url: "example.com")

  |> add_x()

  |> authorize()

  |> add_body()

  |> then(&Req.put_header(&1, "If-Modified-Since", modified_since), if: fn_ -> modified_since end)

  |> then(&Req.put_header(&1, "X-Entity-Id", entity_id), if: fn _ -> entity_id end)

  |> Req.request()

end

```

You can see conciseness is not the point*, but readability, robustness and convenience (a very subjective feeling, so feel free to ignore the last one).

Lastly, I know I could change the code in a million ways to avoid the pattern altogether, maybe even resulting in a cleaner result, but I feel this small addition would be a nice to have, and is something I miss, even if rarely.

* I left the conciseness out of the picture because I think it's way less important, but it does play a bit of a part. The actual example ends up needing to break the `if` into more lines, which doesn't read as good in the middle of the piping.

[Jean Klingler]

Thank you for providing a concrete example.

Also subjective but I find the following more readable

  |> then(&if(modified_since, do: Req.put_header(&1, "If-Modified-Since", modified_since), else: &1))

than

  |> then(&Req.put_header(&1, "If-Modified-Since", modified_since), if: fn_ -> modified_since end)

But perhaps this case might benefit from introducing a private function rather than relying on `then`, especially if this is a recurrent use-case in your module/project:

defp maybe_add_header(req, _key, nil), do: req
defp maybe_add_header(req, key, value), do: Req.put_header(req, key, value)

...

|> maybe_add_header("If-Modified-Since", modified_since)

|> maybe_add_header("X-Entity-Id", entity_id)

It should also be easy define your own then_if/3 macro if you really prefer the clojure style.

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/3c468ed7-1db6-46e3-bf23-45c21e501b3bn%40googlegroups.com.

[Caio Oliveira]

Yup, that’s what I usually do, but I see this in many places. `maybe_put_header`, `maybe_prepend`, etc., which makes me think that an abstraction would be convenient. And yeah, I wrote a `maybe_then` initially, but thought it was simple and convenient enough to be in the kernel.

You received this message because you are subscribed to a topic in the Google Groups "elixir-lang-core" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/elixir-lang-core/uM_M-DWh42A/unsubscribe.
To unsubscribe from this group and all its topics, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CANnyohay83mzBJRgz%3Dy8RZ6cp257u4t8zSfyMKMOfqmSTMvY2A%40mail.gmail.com.

[Jean Klingler]

Coming back to some of my projects, I indeed found a bunch of similar cases, e.g. maybe_put_assoc, so even if I'm still on the fence I do see your point.

Assuming we introduce it, I'd find it more natural API-wise to have the condition first and then the action, which might be why the nested `if` felt more natural to me (consistent with `if`, other conditionals, and also Clojure's cond).

  |> then_if(fn_ -> modified_since end, &Req.put_header(&1, "If-Modified-Since", modified_since))

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJLj4H9%3DsUxyupijoKeJbY1TVYQbjoGF7ALzG9_UT8LF8d655A%40mail.gmail.com.

[Caio Oliveira]

Good idea! I searched through github to find more usecases like that to corroborate this change, and here's what I found:

- 972 results for just `|> maybe_put`: https://github.com/search?q=language%3AElixir+%22%7C%3E+maybe_put%22&type=code

- 214 results for `then(&if...`, where the majority's `else` clause was just `&1`: https://github.com/search?q=language%3AElixir+%22%7C%3E+then%28%22+%26if&type=code

This feels statistically relevant, considering the occurrence of `|> then` is ~6.2k.

I also noticed that most of the uses didn't need the predicate to be a function (and I think in this cases maybe using `|> case do` is even better, and the missing `else` clause won't be a problem here), so maybe this could be even simpler by just taking a value as the condition instead.

Also I'm ok with the `then_if` implementation, but I personally still prefer the `then(..., if: pred)` as it reads better to me, but I understand it might be too different from the rest of the kernel code.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CANnyohZzP5KK-JoXV4BdSg4oGjzgW4AfxOeDu6V7SDqBopR0Ww%40mail.gmail.com.

[Amos King]

I rarely use ‘then’ for anything other than reordering arguments for a pipeline. I find that I end up having the same code that is in the ‘then’ in multiple place and leads me to create a function, with an intention revealing name, instead of using ‘then.’

The new functions lead to more readable and quicker grokking of the code on future passes. I’m always trying to code for future me and future others to speed up understanding of the code. I don’t find that this increases understanding. I think it would lead to more use of it and decrease the amount of time it takes future developers to grok the code.

Amos King

Binary Noggin

On Aug 5, 2024, at 07:48, Caio Oliveira <cai...@gmail.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJLj4H8tdQsK_bpxJXaYQawdLLOsYFt%2BKLL28yDqqbv1rUNP5A%40mail.gmail.com.

[Caio Oliveira]

Hi Amos! Yeah, creating functions with the clear intent is good and preferable. What I don't like is having to create these `maybe_` variants that don't really add any clarity to the code and just serve to wrap a conditional statement and make it "pipeable". Because of that it doesn't feel like the current API is serving to guide devs to writing better code, but just being more verbose.

Also I second your point of rarely using `then`, but as I said before, in that 1% case I need it I think it could be a tad more convenient.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/28FA3E4F-762F-40C9-864B-053CC20F4938%40binarynoggin.com.

[Austin Ziegler]

Related to something else that I was working on, I ran some benchmarks on the performance difference between `apply/3`, calling a captured function, and calling a function directly and the latter was much faster (I want to say it was 10–15% faster, but this was months ago) whereas `apply/3` and captured functions were negligibly different between each other. The proposed `then_if/3` would have one or two captured functions running in the pipeline; if it is something that runs frequently, those would add up.

When I'm building the sort of pipeline described, I don't typically have a `maybe_add_header` *as part of the pipeline*, but instead have something like

…

|> add_modified_since(modified_since)

|> add_entity_id(entity_id)

Those might call `maybe_add_header` inside, or they might just have two (or more) heads that only add the header if `modified_since` and `entity_id` are non-`nil` and non-blank.

I have wanted `{Map,Keyword}.put_if(container, value)` more often, personally (but not frequently enough to want to propose them).

-a

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJLj4H9KN3aN__3YnuD21F%2B9wmzXGp0uE4_9CUUw_zvez2poNA%40mail.gmail.com.

--

Austin Ziegler • halos...@gmail.com • aus...@halostatue.ca
http://www.halostatue.ca/ • http://twitter.com/halostatue

[Caio Oliveira]

Oh, interesting! But I'd imagine in `apply`s case this happens because it needs to figure stuff out at runtime, which shouldn't be the case here, right?

Either way, after thinking about it and looking at `then` usage in the wild I feel like a simpler `condition` term instead of a predicate might be the way to go, so no extra function calls.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJ4ekQuEvC%3D0ptqFOk8TDntk0YLK6pT5wn2mo%3DNRoYd50D6aCQ%40mail.gmail.com.

[Austin Ziegler]

I’d have to find and/or recreate my test cases for this, but `&foo/1` is functionally equivalent to `fn x -> foo(x) end` and to `apply(__MODULE__, :foo, [x])` and it seemed to me that any such capture is treated as a dynamic (runtime resolution) function, acting as a bit of an optimization barrier (the internal function could be optimally performed, but there's no way to inline the behaviour of the internal function).

This could change in the future, but I *think* would require cooperation from the BEAM as well as improvements to the Elixir compiler (that I am in no way qualified to discuss).

Understand that when I say that there was a performance hit, we're still talking about a *massive* IPS value in either case, but it’s worth noting.

I probably wouldn't use either a `then_if/3` as previously proposed or your `then/3` extension, because I think that there are more expressive ways to do this, and even avoid the use of `then/2`. A large part of this is because I used to do things like:

…

|> case do

  …

end

|> case do

  …

end

Which I now consider unreadably complex and would rather see function names over complex `then/2` or `case/2` pipes.

-a

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJLj4H_Yt6-%2B_TkfeNR-q28COp8b4dicopT88CYujWXX4XRM5Q%40mail.gmail.com.

[Caio Oliveira]

Ah, gotcha! Yeah, I was mistakenly thinking of `then` as just a macro and forgot it still relies on a function capture, my bad!

As for your last point, I think the key word there is `probably`. As I said, this is not for the 99% of the cases where a more expressive solution is better, but for the more uncommon cases where we're forced to write a `maybe_*` variant of a function just to play nice with the pipe operator. See the count of a single case of this pattern being used in github (`maybe_put`), which in my opinion points to a possibility of improving the expressiveness of `then`, even if marginally.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJ4ekQuNF-%3Dtsf748N9B81QegR5qMb8p%2BSSHC%3DWtRe0gb5Tthg%40mail.gmail.com.

[Jean Klingler]

Hi Austin,

> I ran some benchmarks on the performance difference between `apply/3`, calling a captured function, and calling a function directly and the latter was much faster (I want to say it was 10–15% faster, but this was months ago)

FYI OTP26 re-introduced an optimization that was broken in OTP24, so the overhead of calling captured functions should actually be very low now:
https://github.com/erlang/otp/pull/6963

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/CAJ4ekQuNF-%3Dtsf748N9B81QegR5qMb8p%2BSSHC%3DWtRe0gb5Tthg%40mail.gmail.com.

[Christopher Keele]

I agree with Greg here, this is clearer:

x |> then(&if(pred(&1), do: f(&1), else: &1))

> your suggestion only implies what happens if `pred` is falsy while mine is clear.

The appeal of the proposed mechanism is its terseness, which mostly comes from this implicit else. My three points against:

- I'm a fan of explicitness in general in the core language.

- We already have an idiom for what implicit elses return in the language—and that's nil, not &1. This mechanism violates that expectation.

- Additionally, eventually someone will want to override the default with a custom else: &capture/1 clause, at which point we're right back to capturing if but using three anonymous functions instead of one.

[Caio Oliveira]

I think it'd be a very weak link between the `if:` option or a `then_if` and the idiom of the actual `if`, especially since this is almost exclusively used in a pipe context. As for the overriding the default value: I think it's a much lower probability of this happening, and I don't think the possibility of someone wanting to extend the API in the future is a good argument for not adding to APIs now: I could argue implementing this extension will make it convenient enough that the chances of someone wanting to extend it again in the future are smaller.

Either way, I think the only way to decide on this is based on personal opinion about what the code looks like with vs. without it, and my opinion is not the popular one, so let's call it :) If anything changes, I'll reopen the discussion.

Thanks everyone for the inputs!

--
You received this message because you are subscribed to a topic in the Google Groups "elixir-lang-core" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/elixir-lang-core/uM_M-DWh42A/unsubscribe.
To unsubscribe from this group and all its topics, send an email to elixir-lang-co...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/fe354ed6-399c-4d24-931b-69f212530204n%40googlegroups.com.