Allow named operations for compatibility with Ecto.Multi #75
Conversation
|
Hi @marioimr, thanks for your contribution! Could you run the elixir formatter on the your changes? Also as it seems CI is not running currently on forked repo. I'll pull and run the tests locally and my initial impression is this is something we can merge. I'll give another consideration soon, thanks again. |
lib/paper_trail/multi.ex
Outdated
| ) do | ||
| model = String.to_atom("model" <> options[:index]) | ||
| version = String.to_atom("version" <> options[:index]) |
There was a problem hiding this comment.
Although I see that these variables wont clash with the model and version references below, it is still in my opinion better to name them differently such as model_key, version_key.
Also it might be a better idea to not have these variables and have them in the options with default values being "model", and "version" instead.
There was a problem hiding this comment.
@izelnakri thanks to you for the library in the first place and for considering my PR.
You're right, it does sound better the naming you suggest, I'll change it shortly, but AFAIK (forgive me I'm a rookie in Elixir) I can't use the ^ for pattern matching with something like options[:model], so I'll keep at least the model variable.
I also ran mix format but it didn't change anything.
changed naming and vars
lib/paper_trail/multi.ex
Outdated
| options \\ [origin: nil, meta: nil, originator: nil, prefix: nil] | ||
| ) do | ||
| def insert(%Ecto.Multi{} = multi, changeset, options \\ [origin: nil, meta: nil, originator: nil, prefix: nil, model_key: :model, version_key: :version]) do | ||
| model = options[:model_key] |
There was a problem hiding this comment.
we can merge this after renaming this to model_key. Thanks!
|
Hi @marioimr, I've merged the PR but tests are failing locally. I'm not going to cut a release unless we fix the tests. Investigating why they are failing now. |
|
this merge introduced a bug on PaperTrail.insert and PaperTrail.insert! APIs, its fixed on this commit: 058a84f I'll cut a 0.8.7 release in few minutes after dependency upgrades. |
|
@izelnakri thank you for the update and the fix! Cheers :-) |
If you're willing to accept it this allows to add a suffix, which defaults to empty string, called "index" in the options so to permit wrapping in a single transactions more operations