Allow uuid generator to be configurable #450
Conversation
FrancisMurillo
force-pushed
the
uuid-generator
branch
2 times, most recently
from
7db5e23
to
8100a15
Compare
FrancisMurillo
force-pushed
the
uuid-generator
branch
from
101a349
to
67d3a7f
Compare
|
I would suggest using https://github.com/bitwalker/uniq and https://github.com/bitwalker/uniq_compat and try to remove |
lib/application.ex
Outdated
|
|
||
| @doc false | ||
| @spec uuid_generator(Commanded.Application.t(), boolean()) :: (() -> uuid) | nil | ||
| def uuid_generator(application, true) do |
There was a problem hiding this comment.
I would have call this method uuid_generator! and removing the last argument
| def uuid_generator(application, true) do | |
| def uuid_generator!(application) do |
There was a problem hiding this comment.
Good suggestion, applied.
| @@ -191,7 +191,8 @@ defmodule Commanded.Assertions.EventAssertions do | |||
| @doc false | |||
| def with_subscription(application, callback_fn, opts \\ []) | |||
| when is_function(callback_fn, 1) do | |||
| subscription_name = UUID.uuid4() | |||
| uuid_generator = Commanded.Application.uuid_generator(application, true) | |||
There was a problem hiding this comment.
You can maybe use only one line here? we don't need uuid_generator afterward
There was a problem hiding this comment.
Okay. Made it more inline
|
|
||
| pipeline = | ||
| pipeline | ||
| |> Map.update!(:command_uuid, uuid_updater) |
There was a problem hiding this comment.
here you could have use Map.put_new_lazy
There was a problem hiding this comment.
Using Map.put_new_lazy does not work on structs since the key is always present and will be a NOOP.
There was a problem hiding this comment.
Oh you right I forgot about the struct!
lib/commanded/commands/router.ex
Outdated
| alias Commanded.Commands.Dispatcher | ||
| alias Commanded.Commands.Dispatcher.Payload | ||
|
|
||
| opts = Keyword.merge(@command_opts, opts) | ||
|
|
||
| application = Keyword.fetch!(opts, :application) | ||
| uuid_generator = Application.uuid_generator(application, false) || fn -> nil end |
There was a problem hiding this comment.
Maybe uuid_generator should return an fn -> nil end instead of just nil ?
There was a problem hiding this comment.
Also updated Application.uuid_generator! returning an a zero arity function
FrancisMurillo
force-pushed
the
uuid-generator
branch
2 times, most recently
from
aa98be2
to
cb38a0e
Compare
|
Removed |
|
I think it was preferable to remove the dependency on |
|
So should have I retained |
Yes, I think so as dropping the dependency removes the potential for version clashes. |
FrancisMurillo
force-pushed
the
uuid-generator
branch
from
cb38a0e
to
445ba8b
Compare
|
Restored |
|
There are two PRs for the same purpose. May I know which one is better to merge? |
Allow uuid generator to be configurable via
Commanded.Applicationuuid_generatoroptions. (#445)One caveat is with dynamic apps. During
Commanded.Middleware.before_dispatch, thecommand_uuidandcorrelation_idis empty which may break some client assumption. For compatibility, it is set afterbefore_dispatch.I do need some guidance on how to update the guide specially this new option and the ecosystem of adapters. Also since
elixir_uuidis optional, the docs might have to specify that as well.