Add DatabaseCleaner::strategy method #518
Conversation
There was a problem hiding this comment.
@rlue Thanks for your first contribution. It looks good!
Could you take a stab at adding coverage to this new method?
You could do that by adding another block to this spec: https://github.com/DatabaseCleaner/database_cleaner/blob/master/spec/database_cleaner/configuration_spec.rb
|
Whoops, I can't get the mysql gem to install on my machine. It's been more or less unmaintained for the past three years, and according to this SO answer, it's not compatible with Ruby > 2.3. Any chance you'd be open to switching to mysql2 to enable development in more modern versions of Ruby? |
|
Ayayay, never mind; I see that mysql2 is already a development dependency as well. Are you able to work on this library in a currently-supported version of Ruby? I went ahead and installed Ruby 2.3.0, but now and according to this SO answer, Ruby 2.2 broke json v1.8.1 (thus json >= 1.8.2 is required for Ruby >= 2.2). I'm open to making any necessary modifications to the library to bring it up to date, but I'm reluctant to undertake the work without your blessing. |
|
@rlue Things have finally improved here, with regards to the dependency issues you were having. If you try to implement this again on current master, you should have a much easier time. |
|
Thanks guys! It's been a while since I've looked at this code; any thoughts on this proposal? |
|
@rlue Hello! Thank you for taking the time to rebase this and add some tests! My personal thoughts are that its a bit awkward for this one getter method to return so many different types of values, but I think that's a natural consequence of the configuration API itself being so awkward. So my first inclination is to merge it. However, there's another consideration: once the dust settles on the v2.0 release, I'd like to pursue a new, non-awkward configuration API (more details in #546), and I imagine that this work might result in us wanting to make breaking changes to this new So I'm a bit torn. @etagwerker do you have any thoughts? |
|
I felt the same way, but tbh I couldn't figure out a better way, so I just submitted what I had. It does feel like a violation of the robustness principle (specifically, the first half—"be conservative in what you send"). If the intention is to modify it soon in the future, I can just put this PR on hold until the 2.0 release. That way, I won't be increasing the API surface that you guys are responsible for maintaining. (It's not like these changes are desperately important, anyway.) |
|
@rlue okay, cool! I want to make improving the configuration API the next order of business after the v2.0 release is out the door, so if you're cool with postponing this API addition so that it can be a part of that endeavor, I think that could work out pretty well. Let's aim for v2.1! |
| @@ -13,6 +13,19 @@ def [](orm, opts = {}) | |||
| fetch([orm, opts]) { add_cleaner(orm, opts) } | |||
| end | |||
|
|
|||
| def strategy | |||
There was a problem hiding this comment.
@rlue I think it would be great if you can document this method. It might be unexpected for users that the strategy method can return nil, a strategy object, or an array.
|
@rlue Now that v2.0 is out, could you please look into this PR? I think it is a good idea but there are now some conflicts with the main branch. |
Issue #511 proposes the addition of a ::strategy getter on the DatabaseCleaner module to aid new users with debugging. This commit introduces that method. Like the ::strategy= setter method, ::strategy is actually defined as an instance method on DatabaseCleaner::Cleaners, and is made available to the top-level module by delegation. Depending on the complexity of the user's configuration, this method may return three different kinds of values: 1. nil (if no strategies have been set on any cleaners); 2. a single strategy (if all cleaners share the same strategy); or 3. a hash of cleaner IDs and the strategies that go with them (if multiple strategies have been set on multiple cleaners). The first of these three cases has not explicitly been tested. === Note This commit also restores a spec context that was removed in c542fee: DatabaseCleaner::Cleaners top level api methods multiple cleaners multiple orm proxy methods with differing orms and dbs This spec context included a single example testing the #orm= method, which was removed in the aforementioned commit. Now, a modified version of the same context is used to test the #strategy method introduced here.
|
Hi folks! Sorry for the delay; hope there is still interest in this PR!
I have not had the chance to really reflect on this concern, but I think it's valid. If we want to follow the robustness principle (that is, to be strict about/consistent in the output that we provide), then rather than supplying potentially three different kinds of output ( def strategy
strategies = transform_values(&:strategy)
endI chose the approach I did based on the assumption that this method would be used primarily for debugging, and thus valued eronomics over consistency. Happy to make amendments as you see fit. |
|
@rlue Thanks for your input! I decided to go with what you said:
I just tweaked that and merged your changes. It should be released in v2.1.0 of database_cleaner. .cc @botandrose |
This PR adds a
DatabaseCleaner::strategygetter method as discussed in issue #511.Since there may be multiple connections, and different strategies for each connection, this getter method returns a hash of strategies.
The guard clause may be unnecessary, since the
::connectionsmethod already attempts to autodetect an ORM and populate the@connectionsarray with it, but I figured better safe than sorry.