a few things #224
a few things #224
Conversation
|
Hi @mayel , thanks for your contributions and making a description/list of changes! It would have been better if these changes were included in separate commits & PRs but I'm currently having a look. 2 comments raised & changes requested. |
lib/paper_trail.ex
Outdated
| def update(changeset, options \\ @default_transaction_options) do | ||
| def update(changeset, options \\ @default_transaction_options) | ||
| def update(%Ecto.Changeset{changes: changes}, _) when changes==%{} do | ||
| {:ok, :no_changes} |
There was a problem hiding this comment.
Ecto.Repo.update today returns {:ok, Ecto.Schema.t()} | {:error, Ecto.Changeset.t()}. Although I do see the benefit of having :no_changes atom return, I'd like to not diverge from the Ecto.Repo.update() return signature whenever possible: https://hexdocs.pm/ecto/Ecto.Repo.html#c:update/2
Can we change this to make it return like Ecto.Repo.update ? I endorse the solution otherwise
There was a problem hiding this comment.
Yeah I wasn't quite happy with that either. What would you suggest instead? I'm not sure if/how we could return the schema when we're given just a changeset, and returning a changeset error seems odd when there wasn't really an error (but just no change to save).
There was a problem hiding this comment.
Ah ecto docs state "by default, if there are no changes in the changeset, update/2 is a no-op" so I guess we can still call it and rely on it to return a schema, but skip inserting the version struct in PaperTrail.Multi.update/3 if the changeset's changes is empty...
There was a problem hiding this comment.
Ah ecto docs state "by default, if there are no changes in the changeset, update/2 is a no-op" so I guess we can still call it and rely on it to return a schema, but skip inserting the version struct in
PaperTrail.Multi.update/3if the changeset'schangesis empty...
yes
There was a problem hiding this comment.
@izelnakri done, that worked well :)
izelnakri
force-pushed
the
main
branch
6 times, most recently
from
ec182bc to
c9de130
Compare
Hi, hope this helps!