Use destroy over delete when deleting translation records

[mpataki]

Ensures that rails callbacks get called on destroy.

[timfjord]

@mpataki are there any specific reasons why we need to use destroy over delete?

[gr8bit]

For the sake of possible callbacks in Translation model I guess. Not the worst idea imho, even if it might impact performance as models are fetched, instantiated and deleted one by one...

[mpataki]

Oof, sorry for the ulta-late reply on this. I'm just working out better github notification so things like this don't get lost in the noise.

Using destroy over delete opens up the option to use rails callbacks to react to the destroy event. In my case, using a fork of this repo which included this change, I used it to track these destroy events in one environment so I could verify and replicate them on others, but of course there could be many other uses.

As for the performance concern, I leave that to you guys. I know that in my use cases it never had any impact as we aren't really storing new translations on the fly, just from our internal admin panel as people add / update language, but I can't really speak for all users.

Thanks for having a look! I'll be sure to be more responsive here in the future.

[timfjord]

@mpataki Thanks for explanation.
And I think this PR definitely make sense, but I would rather make it optional.
So to use destroy over delete you just need to configure it somewhere

[mpataki]

That sounds reasonable. I'll put something together. Thanks!

[timfjord]

You can add some simple configuration code here: lib/i18n/active_record.rb
And in the end we can use it:

I18n::ActiveRecord.configure do |config|
  config.cleanup_with_destroy = true
end

[mpataki]

Ok, this should do it. I ended up adding the configuration to I18n::Backend::ActiveRecord instead of I18n::ActiveRecord as it seemed to be more in line with what you have going on here already, but I'm happy to re-work it so it's in lib/i18n/active_record.rb if you'd still prefer. I've also added some tests, version bumped, and updated the readme.

One thing - I've defaulted the configuration to using delete_all. I just wanted to point that out for transparency.

Let me know if there's anything further you'd like to see changed!

[timfjord]

Let's use @config instead

[mpataki]

This being a class variable, I don't think that's allowed by ruby. I get this:

NameError: `@config' is not allowed as a class variable name

[timfjord]

Let's go with @_config then

[mpataki]

Yeah, same thing. Investigating a little further, it's class_variable_set that's throwing it, and otherwise it's allowed to be @config (one @ instead of two). I was aiming to avoid exposing a writer for the config on ActiveRecord, but exposing one and using it over class_variable_set in the tests accomplishes what you're asking for, if you don't mind the setter being there.

I'll make that change and push so you can see it.

[timfjord]

@mpataki PR looks nice, i added one notice.
Can you please also squash all commits

[timfjord]

No need to pass &block here, since we using yield and block_given?

[timfjord]

Cool, great work @mpataki, now we have configuration api.
Thanks for contribution!