search

smpallen99 / whatwasit

Track changes to your Ecto models
MIT License
63 stars 8 forks source link

Violation of changeset design principals? #4

Closed joshgoebel closed 8 years ago

joshgoebel commented 8 years ago

Isn't the concept of a changeset that it's in preparation for a change? IE, you create the changeset then later you apply it.. it seems by suggesting users add prepare_version to their models changeset method that you're now making calling changeset have side-effects (writing to the DB), where-as before (intentionally) it had none.

Could be I'm misunderstanding the concept, but I don't think so. I feel like we need yet another function to encapsulate the concept of versioning without piggy-backing on changeset itself.

joshgoebel commented 8 years ago

I feel like the right injection point might be a new Repo module or wrapper function around the repo itself vs the changeset...

changeset = Model.changeset(data)
VersioningRepo.insert!(changeset)

or

changeset = Model.changeset(data)
TrackChanges.insert!(changeset, MyApp.Repo)

Just some thoughts.

smpallen99 commented 8 years ago

My original attempt was to override the insert and delete actions on the repo. José’s feedback was to use the changeset, so that is what I did.

I don’t think there is anything stopping someone one doing at the repo level. Just don’t use the prepare_version api. The basic APIs should be available to do the inserts at the repo level.

Steve

On Jul 23, 2016, at 3:04 PM, Josh Goebel notifications@github.com wrote:

I feel like the right injection point might be a new Repo module or wrapper function around the repo itself vs the changeset...

changeset = Model.changeset(data) VersioningRepo.insert!(changeset) or

changeset = Model.changeset(data) TrackChanges.insert!(changeset, MyApp.Repo) Just some thoughts.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/smpallen99/whatwasit/issues/4#issuecomment-234734569, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2YpeWXMHGTptWMF8qUG-3sCUvhAgHIks5qYmXJgaJpZM4JTa52.

joshgoebel commented 8 years ago

I pinged him on Twitter with my question. It just seems like you're making (suggesting) a change to changeset() that entirely alters the behavior of it.

I'm very curious to see the "right" way to do this in Elixir (it's the exact reason i came to read this code in the first place), but this doesn't feel like it. And imagine we start to have a bunch of other "plugins" that do things with outside side effects (send an email, text, etc)... is the recommendation really to put them ALL inside changeset?

joshgoebel commented 8 years ago

Also for most systems shouldn't we really include a transaction in the examples? What about something like:

Repo.transaction fn ->
  changeset = Model.changeset(model, new_data)
  |> Model.save_prior_version!
  |> Repo.insert!
end
smpallen99 commented 8 years ago

@yyyc514 Actually, there are no side effects in the changeset. prepare_versions uses the Changeset.prepare_changes api. It is passed an ann fun that gets called during the Repo action. If the Repo action fails, the prepare_changes function does not get called. So, there is no need for a transaction.

If you read the docs for prepare_changes you'll see that it is designed for this type of post processing.

josevalim commented 8 years ago

So to clarify: changesets are split in two stages. The casting, validation, preparing are first and are side-effect free. Then, once you send it to the repository, all side-effects happen. This includes inserting to the database, running prepare_changes, and setting up transactions (if prepare changes or associations are included). So I wouldn't say changesets are side-effects free but rather they split the stages with side-effects and without side-effects in two very clear steps. There is still a lot of benefit in splitting, you get to have completely sane and immutable semantics until the database comes into action. Btw, the transaction is automatically taken care of if you have associations or set prepare_changes.

joshgoebel commented 8 years ago

Changeset.prepare_changes Ah that is what I was somehow missing. Thank you! Now it's all so very clear. :-) Closing issue.

joshgoebel commented 8 years ago

@smpallen99 Sorry for the confusion. :)

smpallen99 commented 8 years ago

Josh,

No problem. It was a good discussion. I appreciate José’s description of the two stages of a changeset.

Steve

On Jul 24, 2016, at 12:32 PM, Josh Goebel notifications@github.com wrote:

@smpallen99 https://github.com/smpallen99 Sorry for the confusion. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/smpallen99/whatwasit/issues/4#issuecomment-234786933, or mute the thread https://github.com/notifications/unsubscribe-auth/AA2YpTEX044AvKcdi-dBxIaAZtmYi8ckks5qY5OIgaJpZM4JTa52.