Skip yielding when no block provided #213
Conversation
2 similar comments
lib/mongoid/history/trackable.rb
Outdated
| @@ -282,6 +282,8 @@ def track_history_for_action(action) | |||
|
|
|||
| clear_trackable_memoization | |||
|
|
|||
| return unless block | |||
There was a problem hiding this comment.
The ruby way is to do unless block_given? (which I believe is much faster) and to mark the block above as _block.
There was a problem hiding this comment.
Good feedback, thanks!
There was a problem hiding this comment.
Turns out, with block_given? there's no need for a &block param!
spec/unit/trackable_spec.rb
Outdated
| @@ -621,6 +621,19 @@ def self.name | |||
| end | |||
| end | |||
|
|
|||
| describe '#track_history_for_action' do | |||
| before(:all) { MyModel.track_history } | |||
| let!(:m) { MyModel.create!(foo: 'bar') } | |||
There was a problem hiding this comment.
You don't need a ! here, nitpick.
3 similar comments
| expect { m.send(:track_history_for_action, :update) }.not_to raise_error | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
I think you should rewrite the tests at a higher level, so not calling track_history_for_action via a send, but wherever this is actually used, because that is an internal function and we want to make sure the code works at the public interface level.
| @@ -282,6 +282,8 @@ def track_history_for_action(action) | |||
|
|
|||
| clear_trackable_memoization | |||
|
|
|||
| return unless block_given? | |||
There was a problem hiding this comment.
Do add &_block as a parameter in the function definition to make it clear that this function allows for a block.
| @@ -5,6 +5,7 @@ | |||
| * [#205](https://github.com/mongoid/mongoid-history/pull/205): Allow modifier field to be optional - [@yads](https://github.com/yads). | |||
| * [#211](https://github.com/mongoid/mongoid-history/pull/211): Enable tracking create/destroy by default - [@jagdeepsingh](https://github.com/jagdeepsingh). | |||
| * [#212](https://github.com/mongoid/mongoid-history/pull/212): `track_history` method support for `:if` and `:unless` options - [@jagdeepsingh](https://github.com/jagdeepsingh). | |||
| * [#213](https://github.com/mongoid/mongoid-history/pull/213): Skip yielding when no block provided - [@mikwat](https://github.com/mikwat). | |||
There was a problem hiding this comment.
It's unclear what is no longer yielding, so should say Do not yield on ... when no block is provided or something like that.
|
Since this is an interface change I would expect something to be updated in README, too. |
|
Thanks for the feedback @dblock I'm realizing the root problem appears to be that |
Normally
track_history_for_actionis always called with a block viaaround_create/update/destroy, but I've found in my own code base that there are times when it's desirable to call this method directly without a block.Regardless, this change seems like good hygiene with little downside. If a block isn't provided, then there's no need to rescue and rollback the new history track.