Improve disable and disable_tracking, add their inverses
#240
Conversation
`Mongoid::History.disable` and `disable_tracking` now restore the original state if given a block and can be run without a block plus `Mongoid::History.enable` and `enable_tracking` have been added.
lib/mongoid/history.rb
Outdated
| def disable(&_block) | ||
| store[GLOBAL_TRACK_HISTORY_FLAG] = false | ||
| yield | ||
| def disable(&block) |
There was a problem hiding this comment.
I feel that disable! and enable! that doesn't take a block along with enable and disable that does would be more idiomatic. In Ruby we want the developers to write "english", not pass in true/false. Then you don't need the track(boolean) method at all.
lib/mongoid/history/trackable.rb
Outdated
| with_tracking(true, &block) | ||
| end | ||
|
|
||
| def with_tracking(state) |
There was a problem hiding this comment.
Similar to the class methods, these should be called with_tracking and without_tracking. I would alias disable_tracking to without_tracking and never expose a method that has a boolean parameter. It's OK to duplicate code a little bit too IMO for something so trivial.
|
I only added track/with_tracking as a way to reduce duplication, which is why I didn't test them directly or document them. I thought about prefixing them with an underscore. I did think about having bang methods because that did seem nicer to me, but then there's duplication again and really the current non-bang block-or-no-block interface is very similar to Ruby's What do you think about just removing track/with_tracking? |
I'm open to it. Try it let's see what it looks like? |
CHANGELOG.md
Outdated
| @@ -4,6 +4,7 @@ | |||
| * [#236](https://github.com/mongoid/mongoid-history/pull/236): Fix Ruby 2.7 keyword argument warnings - [@vasilysn](https://github.com/vasilysn). | |||
| * [#237](https://github.com/mongoid/mongoid-history/pull/237): Fix tracking subclasses with additional fields - [@getaroom](https://github.com/getaroom). | |||
| * [#239](https://github.com/mongoid/mongoid-history/pull/239): Optimize `modified_attributes_for_create` 6-7x - [@getaroom](https://github.com/getaroom). | |||
| * [#240](https://github.com/mongoid/mongoid-history/pull/240): `Mongoid::History.disable` and `disable_tracking` now restore the original state if given a block and can be run without a block plus `Mongoid::History.enable` and `enable_tracking` have been added - [@getaroom](https://github.com/getaroom). | |||
There was a problem hiding this comment.
Split in two lines with the same PR. There are two changes here.
README.md
Outdated
| @@ -149,11 +149,34 @@ Comment.disable_tracking do | |||
| comment.update_attributes(:title => "Test 3") | |||
| end | |||
|
|
|||
| # globally disable all history tracking | |||
| # disable tracking for comments by default | |||
| Comment.disable_tracking | |||
There was a problem hiding this comment.
Let's alias disable_tracking to disable_tracking! and document the latter as the recommended way to do this. So either
Comment.disable_tracking!or
Comment.disable_tracking do
...
end
README.md
Outdated
| Mongoid::History.disable do | ||
| comment.update_attributes(:title => "Test 3") | ||
| user.update_attributes(:name => "Eddie Van Halen") | ||
| end | ||
|
|
||
| # globally disable all history tracking by default | ||
| Mongoid::History.disable |
There was a problem hiding this comment.
Same as the above, disable! and enable!
|
👍 |
Mongoid::History.disableanddisable_trackingnow restore theoriginal state if given a block and can be run without a block plus
Mongoid::History.enableandenable_trackinghave been added.I was originally going to use these to globally disable tracking in our test suite and enable it selectively for individual tests, but we have request_store in our app and therefore the value got reset by individual requests. I ended up just stubbing
Mongoid::History.enabled?instead.