assaf / vanity

Experiment Driven Development for Ruby
http://vanity.labnotes.org
MIT License
1.55k stars 269 forks source link

Invoke `on_assignment` callback after assignment is stored in database; add global `on_assignment` callback #368

Closed frostmark closed 2 years ago

frostmark commented 2 years ago

MVP for #367

Edit: final version of this PR does not include an after_assignment callback. Instead, the on_assignment callback's call has been moved after the assignment is preserved in the database.

frostmark commented 2 years ago

@bensheldon could you also take a look at this? 👀

bensheldon commented 2 years ago

Just to summarize what I'm seeing here.

Reading the original code, I want to suggest a completely different direction too:

frostmark commented 2 years ago

@bensheldon

Change the behavior such that the Experiment Callback overrides the global Configuration callback and only one is called?

Yep, it's already working that way. Default callback can be overridden in Experiment

Keep the global on_assignment (I think having either a default or additional callback sounds useful)

Don't create a separate after_assignment callback and instead we simply move the place where on_assigment is invoked till after it's saved in the database. Is there a functional/behavioral reason to have both callbacks? I did a little digging and didn't really see an explanation for why it should take place before save (i.e. it's not cancelable, there isn't anything subsequent that mutates the value).

Yep, I agree that we may don't add after_assignment. I just didn't want to change the current behavior of on_assignment, but I agree that there is no need to add a new type of callback and we can move on_assigment

bensheldon commented 2 years ago

@frostmark this looks good. Let me know when I should review.

frostmark commented 2 years ago

@bensheldon this is ready for review

bensheldon commented 2 years ago

Released in v3.1.0