chromic_pdf icon indicating copy to clipboard operation
chromic_pdf copied to clipboard

Support `for_option` in the protocols macros

Open GuillaumeMilan opened this issue 1 year ago • 4 comments

Description of the feature

At the moment it is not possible to create operations you want to repeat in the protocols. This could be handy for implementing set_cookies option to be able to set multiple cookie on the printing session.

Example:

defmodule MyApp.ProtocolExample do

  steps do
    for_options :set_cookies, ext:  :acc do
      call(:set_cookie, "Network.setCookie", &Map.fetch!(&1, :set_cookies_acc), %{httpOnly: true})

      await_response(:cookie_set, [])
    end
end

Implementation consideration

To be able to implement such a feature I am questioning myself on the following points:

  • Why steps are done through macros and not functions? While they are converted to function calls at the end
  • For the accumulator shall we save it inside the state? And if yes is there some standard to name keys (in the example I put it as :set_cookies_acc as the concatenation on set_cookies and acc as the extension.)

Also for the moment all the protocols system is not documented. Will it be part of a next iteration? (I am thinking it could be nice to make it part of a separated library)

GuillaumeMilan avatar Oct 27 '24 18:10 GuillaumeMilan

Hi @GuillaumeMilan

thank you for your request. I'm rather hesitant to put further complexity into ProtocolMacros, TBH, hence may I ask

This could be handy for implementing set_cookies option to be able to set multiple cookie on the printing session.

Is this an actual requirement of yours or are you just looking to contribute?


To your questions:

Why steps are done through macros and not functions? While they are converted to function calls at the end

There's probably no good reason for this. I wanted some syntax sugar and be able to write my "protocols" as almost linear code. But one could have easily implemented this in a functional way as way and come out with a similarly ok-ish looking interface.

%Protocol{}
|> Protocol.add_step(:call, &my_call_fun/2)
...

For the accumulator shall we save it inside the state? And if yes is there some standard to name keys (in the example I put it as :set_cookies_acc as the concatenation on set_cookies and acc as the extension.)

That would be the easiest, sure. I think I'd just re-use the set_cookies name as the iterator, because why not.

You have something like this in mind, right?

      defp do_build_steps([{:for_option, key} | rest], acc, opts) do
        for value <- List.wrap(Keyword.get(opts, key)) do
          do_build_steps_until_end(rest, acc, Keyword.put(opts, key, value))
        end
        |> List.flatten()
        |> ++(do_build_steps(skip_branch(rest, ...))
      end

maltoe avatar Oct 28 '24 08:10 maltoe

Is this an actual requirement of yours or are you just looking to contribute?

This is indeed a requirement I have in a project. But to do so I just created a custom protocol in charge of binding once a cookie. And then use the regular flow to bind the second one. My worry is as the protocol API is not documented it can be interpreted as a private API. And so might change in future version of the library.

I would also be happy to participate in the project but to be able to do so I will need to understand some of the choices made first.


There's probably no good reason for this. I wanted some syntax sugar and be able to write my "protocols" as almost linear code. But one could have easily implemented this in a functional way as way and come out with a similarly ok-ish looking interface.

Ok clear I'll try to think about it and come with a proposition that could keep the current protocols syntaxe but providing additional features

That would be the easiest, sure. I think I'd just re-use the set_cookies name as the iterator, because why not.

At first I was thinking the same, but then I though what if the user wants to use the complete opts argument along one. If we destructure inside the same name then it will not be possible. (:as can stay an option of for_option and only destructure in other place if requested)

You have something like this in mind, right?

Yes something similar

GuillaumeMilan avatar Oct 28 '24 09:10 GuillaumeMilan

My worry is as the protocol API is not documented it can be interpreted as a private API.

I don't intend to make it public as I don't consider it "essential" and it's not really polished either. That said, it's also unlikely that the protocol "virtual machine" is going to change anytime soon.

At first I was thinking the same, but then I though what if the user wants to use the complete opts argument along one. If we destructure inside the same name then it will not be possible. (:as can stay an option of for_option and only destructure in other place if requested)

OK, please do an :as option then (instead of ext and name interpolation), and let it default to the same key.

maltoe avatar Oct 28 '24 09:10 maltoe

Planning to work on this at the end of the week. Hope I can sort it out during the WE

GuillaumeMilan avatar Oct 29 '24 09:10 GuillaumeMilan