MULTITRANS

[KBWilliamP]

[MULTITRANS] add new type of transition to be able to do a transition on multiple states or not

[half-shell]

Thanks for the PR, nice to see some activity on this repo 😃

If I understood clearly, the current need is to avoid having to implement several transitions when the same action for several states need to be declared.
In the example case you wrote, that would translate to:

# With the `multitrans` macro
defmultitrans refund({:included, [:payed, :shipped, :partially_shipped], _}, order), do: [...]

# Would otherwise result in
deftrans payed({:refund, _params}, state), do: [...]
deftrans shipped({:refund, _params}, state), do: [...]
deftrans partially_shipped({:refund, _params}, state), do: [...]

Which can understandably get pretty crowded, even if those transitions have a single function called.

However, while the feature would be welcome, I am unsure we'd need to separate those transitions from the others. I think we'd still benefit from keeping and reusing the internal transitions, as it helps us leverage all the utility functions already there, but also prevents us from having another "special case" when it really just is the same thing as far as logic goes. To me it really just is an interface issue.

My proposal would be for the defmultitrans macro to "generate" actual transitions in the @fsm map, instead of having a separate @multitrans map.
That would also allow us not to have another nested case bloc in the event/2 function, and would keep the flow of exfsm as it is, which is a net positive considering the stability requirement for this lib.

IMO, not only to make the interface simpler, but also to keep transitions readable, I'd get rid of the mode all-together, and just have the :included behavior being the only one. While I understand why the :excluded mode might come in handy, I think I'd be in very few cases, and the upside handiness does not feel worthwhile.

This obviously is a conversation, so feel free to argue over any of my points! I'm pretty sure others will have things to say as well.

[Shakadak]

IMO, not only to make the interface simpler, but also to keep transitions readable, I'd get rid of the mode all-together, and just have the :included behavior being the only one. While I understand why the :excluded mode might come in handy, I think I'd be in very few cases, and the upside handiness does not feel worthwhile.

I'm in favor of that, especially when the FSM becomes hairy, I'd rather be aware of the complexity

My proposal would be for the defmultitrans macro to "generate" actual transitions in the @fsm map, instead of having a separate @multitrans map.

I also agree with that, though if we still keep the :excluded mode, it should be done in the before compile stage

[Shakadak]

Is there a reason you don't handle the to attribute ? like with deftrans

[KBWilliamP]

I just took the defbypass as model because at the beginning it was more similar to the defbypass.
There isn't any particular reason.

[Shakadak]

This can be compressed into

Suggested change

	(handlers |> Enum.map(&(&1.event_multitrans)) |> Enum.concat |> Enum.into(%{}))
	Map.new(Enum.flat_map(handlers, &(&1.event_multitrans)))

[KBWilliamP]

We can do it for the def fsm and def event_bypasses as well

[Julien29121998]

Thanks for your PR @KBWilliamP

About the Toolbox's point of view:
I fully agree with this:

However, while the feature would be welcome, I am unsure we'd need to separate those transitions from the others. I think we'd still benefit from keeping and reusing the internal transitions, as it helps us leverage all the utility functions already there, but also prevents us from having another "special case" when it really just is the same thing as far as logic goes. To me it really just is an interface issue.

=> I think the only reason @KBWilliamP kept them separate is to be able to deal with the excluded/included parameter.
In any case, it still might make the debugging more complex to have to check the two structures (the multitrans and fsm map) while everything could be in the same one (fsm map). Additionnaly, it makes the maintenance and potential improvements of this library more difficult, as it would mean to duplicate any change to each one of the two parallel structures.

Moreover, as the others already mentionned, I'm not sure that there are a lot of cases where the excluded option could be used.
Often, in projects, you encounter the included case, where you might want to collapse three or four similar transitions into one, but I've never seen a case where there are so many states in my fsm and where there is a transition that starts from almost all of them (This type of fsm could be seen as badly implemented, actually).

It is always better to have control over an exhaustive list of starting states than to use a complementary approach: imagine if you use excluded and then someone adds a new state, without being aware of your use of this parameter: they will end up with an unwanted transition. Could be dangerous...

My proposal would be for the defmultitrans macro to "generate" actual transitions in the @fsm map, instead of having a separate @multitrans map. That would also allow us not to have another nested case bloc in the event/2 function, and would keep the flow of exfsm as it is, which is a net positive considering the stability requirement for this lib.

=> for short, I fully agree with the others, you should keep the core of your genius idea, because it will simplify developper's lives. However, make it so that it feeds the @fsm map, to improve debug capacity and facilitate future evolution & maintenance; and to preserve the mental health of future developpers and the integrity of our projects, drop the exclude.

Feel free to further comment and discuss this

[KBWilliamP]

Thank you for your feebacks, it has been done quickly, so I didn't take in count everything, like the @to
I understand the fact that adding a new @multitrans object to do pretty much the same isn't so good, I didn't liked it either but to do the excluded quickly I did it like this.

Moreover, as the others already mentionned, I'm not sure that there are a lot of cases where the excluded option could be used. Often, in projects, you encounter the included case, where you might want to collapse three or four similar transitions into one, but I've never seen a case where there are so many states in my fsm and where there is a transition that starts from almost all of them (This type of fsm could be seen as badly implemented, actually).

In fact there could be some case where the :excluded could be usefull.
=> Imagine a project with a lot of state from the creation of the order, to the shipping of the order. Now you have to add a refund state, since you got the order when it has been already paid, every state can be refunded except if it has already been refunded.

It is always better to have control over an exhaustive list of starting states than to use a complementary approach: imagine if you use excluded and then someone adds a new state, without being aware of your use of this parameter: they will end up with an unwanted transition. Could be dangerous...

This exemple works in the other case too, if you want to add a new transition without knowing that you have to add it to the list of already existing multitrans

=> for short, I fully agree with the others, you should keep the core of your genius idea, because it will simplify developper's lives. However, make it so that it feeds the @fsm map, to improve debug capacity and facilitate future evolution & maintenance; and to preserve the mental health of future developpers and the integrity of our projects, drop the exclude.

Even though the excluded can be usefull I agree that we can do it by just using the @fsm already existing.

[KBWilliamP]

The problem is here.
In the case we want to create a defmultitrans that just create a lot of object in @fsm how can we know in the find_multitrans when we have to apply on a defmultitrans or on a deftrans and how to find back the right states it had at the beginning.
We need to make the @fsm more complex.

[half-shell]

The goal brought up beforehand should mean that we do not need a find_multitrans/1 anymore, because we can rely on the regular find_handler/1. Here's a quick shot at what I think the macro would look like:

defmacro defmultitrans(
             {event, _meta, [{states, _params} | _rest]} = signature,
             body_block
           ) do
    quote do
      @fsm Map.merge(
             @fsm,
             Enum.map(
               unquote(states),
               fn state ->
                 {
                   {state, unquote(event)},
                   {__MODULE__, unquote(Enum.uniq(find_nextstates(body_block[:do])))}
                 }
               end
             )
             |> Map.new()
           )
      [...]
      def unquote(signature), do: unquote(body_block[:do])
      @to nil
    end
  end

You're also more than welcome to add a new instance in the ExFSM's doctest to keep it updated. The Door module works somewhat well to express the multitrans idea:

defmodule Elixir.Door do
  use ExFSM

  defmultitrans open({[:closed, :opened], _}, s), do: {:next_state, :opened, s}
end

I might try to setup github actions to run a simple mix test

[half-shell]

To address my earlier misunderstanding regarding the fact that we'd still need a find_multitrans/1 in order for ExFSM.Machine.event/2 to apply the arguments to the right function:
I'd argue we'd be better off by just straight up generating the functions that'd be creating if we were using deftrans instead of multitrans:

defmacro defmultitrans(
             {event, _meta, [{states, _params} | _rest]} = signature,
             body_block
           ) do
  quote do
     [...]
     unquote(
       for state <- states do
         quote do
           def unquote({state, meta, [{trans, params} | rest]}), do: unquote(body_block[:do])
         end
       end
     )
      @to nil
    end
  end

What do you think?

[Shakadak]

IMO, what we want internally is a way to transpose transition <-> state in the signature. If we move the code generation from deftrans to a dedicated function, the code can be shared between deftrans and defmultitrans, then it just becomes a matter of converting the input from defmultitrans into what would look like the inputs for multiple deftrans
Originaly I wanted to reuse deftrans directly in a quote to reduce the amount of code changed, but from experience, handling the ast in a function for reuse purpose has always been better than using quote and the macro.

But then the @to can't be accessed from inside the macro even via Module.get_attribute/3 because the expansion time of a macro happen before the evaluation time of the module's ast
So the logic to clean up the @to attribute needs to be separate from the logic to generate the transition ast

In the end I came up with:

  defmacro deftrans(signature, do: body) do
    transition_ast(signature, body)
    |> cleanup_to()
  end

  defmacro defmultitrans(signature, do: body) do
    Enum.map(transpose(signature), &transition_ast(&1, body))
    |> cleanup_to()
  end

where transpose, transition_ast, and cleanup_to are:

  @doc false
  # states to transitions
  def transpose(signature) do
    {transition, meta, params} = signature
    [{states, input}, object] = params
    Enum.map(states, fn state -> 
      {state, meta, [{transition, input}, object]}
    end)
  end

  @doc false
  # Define the function for the transition, as well as it's metadatas
  def transition_ast(signature, body) do
    {state, _meta, params} = signature
    [{transition, _param}, _object] = params
    quote do
      @fsm Map.put(@fsm, {unquote(state), unquote(transition)}, {__MODULE__, @to || unquote(Enum.uniq(find_nextstates(body)))})
      doc = Module.get_attribute(__MODULE__, :doc)
      # I don't really understand why we don't directly use @doc
      @docs Map.put(@docs, {:transition_doc, unquote(state), unquote(transition)}, doc)
      def unquote(signature), do: unquote(body)
    end 
  end

  @doc false
  # Remove the @to for the next transition definitions
  def cleanup_to(ast) do
    quote do
      unquote(ast)
      @to nil
    end
  end

---

I really think we should wait another time to do the difference operation on the set of transitions. As stated, it is a source of implicit behaviour dependent on the global definition of the fsm (which may be more than the current module), which is really easy to forget when changing stuff.

[KBWilliamP]

I really like your implementation that you describe here, I tried it but we have a problem here:

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def to_be_orchestrated/2" was previously defined (lib/fsm/orders.ex:149)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def partially_shipped/2" was previously defined (lib/fsm/orders.ex:230)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def no_automatic_orchestration/2" was previously defined (lib/fsm/orders.ex:186)
  lib/fsm/orders.ex:241

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def to_be_orchestrated/2" was previously defined (lib/fsm/orders.ex:149)
  lib/fsm/orders.ex:245

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def partially_shipped/2" was previously defined (lib/fsm/orders.ex:230)
  lib/fsm/orders.ex:245

warning: clauses with the same name and arity (number of arguments) should be grouped together, "def no_automatic_orchestration/2" was previously defined (lib/fsm/orders.ex:186)
  lib/fsm/orders.ex:245

the definition of the new deftrans has they are already existing is a problem.
Right now I don't have any solution...

[Julien29121998]

@KBWilliamP actually, this is only a matter of warning:
You get this error because the clauses defined by deftrans and defmultitrans that are originating from the same state are not grouped together.
However, it still compiles properly.
Here, you have two choices, i guess:

• Finding a way to desactivate the warning with something like Code.compiler_option during the compilation of the module
• Storing all the AST of your transitions in a variable and then sorting it (based on the origin state) so that the clauses are grouped together, before unquoting it as functions.

Additional remarks:

• The defmultitrans declaration is inconsistant: when we use deftrans, we write

deftrans [original state]({:[action], [params]} [object])

whereas when using defmultitrans, you put:

defmultitrans [action] ...

Starting with the action, which can be misleading, as junior developper already tend to get mixed up on the syntax.
I'll suggest that you find a different way to declare multitransition, as you cant start with the states (obviously) but can't start with the action either (not advised).

Finally I would suggest keeping the original guards in the deftrans macro:

defmacro deftrans({state,_meta,[{trans,_param}|_rest]}=signature, body_block) do

[half-shell]

Sorry for the delay @KBWilliamP

After talking a bit with @awetzel , it turns out that there might be a major API change some time this year for ExFSM.

Taking that into account, the changes we were willing to make for the defmultitrans feature to get implemented as we'd wish probably won't be worth it on a short term timeline.

That means that we agreed that in order to get this QOL feature out and usable quickly enough, the easiest things to do is just to mute the compilation warning, using the quote option generated: true (c.f. https://hexdocs.pm/elixir/Kernel.SpecialForms.html#quote/2-options)