-
Notifications
You must be signed in to change notification settings - Fork 243
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
Allow uuid generator to be configurable #450
base: master
Are you sure you want to change the base?
Conversation
7db5e23
to
8100a15
Compare
101a349
to
67d3a7f
Compare
I would suggest using https://github.com/bitwalker/uniq and https://github.com/bitwalker/uniq_compat and try to remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello and thank's for this amazing PR !
I've made some suggestions but I'm also concern about the UUID
module which seems to be unnecessary as pointed by @yordis
Maybe can we completely remove it as suggested?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can maybe use only one line here? we don't need uuid_generator
afterward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Made it more inline
|
||
pipeline = | ||
pipeline | ||
|> Map.update!(:command_uuid, uuid_updater) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here you could have use Map.put_new_lazy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe uuid_generator
should return an fn -> nil end
instead of just nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also updated Application.uuid_generator!
returning an a zero arity function
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. |
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.Application
uuid_generator
options. (#445)One caveat is with dynamic apps. During
Commanded.Middleware.before_dispatch
, thecommand_uuid
andcorrelation_id
is 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_uuid
is optional, the docs might have to specify that as well.