Be more surgical about unscoping #265
Conversation
We now only remove the `where` scopes, and use `reorder` where necessary to ensure our ordering is paramount. Rails 3.2 compatibility is maintained via the `except` method instead of `unscope`.
|
Bummer. Looks like Rails 3.2 isn't working yet. I'll keep going. |
|
Sadly, in Rails 3.2 I was close to dropping 3.2 support but realised we could still continue, but just fork the code (as we were already doing anyway) for 3.2 so that it used our existing model of All tests should pass (they do locally) so are you happy for me to merge this change on a minor version? The only functionality change might be that people are incorrectly relying on the unscoping but I doubt it. |
|
Proposal: For all the 3.2 and backwards compatibility |
| @@ -48,6 +48,7 @@ def teardown_db | |||
|
|
|||
| require "shared" | |||
|
|
|||
| # require 'logger' | |||
There was a problem hiding this comment.
Is this an oversight? We could just remove it otherwise.
There was a problem hiding this comment.
For some reason logger wasn't being loaded when I was running the tests locally with Appraisal and the Rails 3.2 gemset. Sometimes it's handy to be able to see the SQL logs when debugging the tests so I've left this here commented out to save searching for the solution again in the future.
|
RE the Mixin, that sounds like a good idea. I'll have a go at that now. |
|
Actually, following this up. There really isn't anywhere else where we're checking the rails version for Rails 3.2. Mostly it's checking for things like > 5. The only other Rails 3 check is when dealing with detecting destroy via a parent (that we added recently) and we're just skipping that functionality for Rails 3. I'm at a loss as to where to start given some of this code is within the meta-code 'definers'. I'm happy to work on it, but need a bit more assistance with how to structure it. Otherwise, we can commit this as is, then draw a line in the sand and the next time a code branch is required for Rails 3.2 we'll just drop support. What do you think? |
|
On Tue, 4 Apr 2017 at 3:15 AM, Brendon Muir ***@***.***> wrote:
Actually, following this up. There really isn't anywhere else where we're
checking the rails version for Rails 3.2. Mostly it's checking for things
like > 5. The only other Rails 3 check is when dealing with detecting
destroy via a parent (that we added recently) and we're just skipping that
functionality for Rails 3.
I'm at a loss as to where to start given some of this code is within the
meta-code 'definers'.
I'm happy to work on it, but need a bit more assistance with how to
structure it.
Otherwise, we can commit this as is, then draw a line in the sand and the
next time a code branch is required for Rails 3.2 we'll just drop support.
What do you think?
Yeah, that sounds good to me.
… —
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#265 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFjGFopdyZjVoPePqxVgBNH9GFULVSsks5rsWiIgaJpZM4MuI4Q>
.
|
Related to: brendon/acts_as_list#265 I’ve been running into deadlocks in my app. In the past I’ve identified these being caused by `acts_as_list` being too greedy in its unscoping (removing default order scopes, and thus creating situations where the table is searched from opposite ends resulting in a deadlock). Ancestry seems to be doing the same thing. I’ve made a modification that ensures we only remove the `:where` scope, leaving other scopes (like `:order`) intact. The tests still pass (locally). I’ve added in a workaround for Rails 3.2 that doesn’t support this functionality.
We now only remove the
wherescopes, and usereorderwherenecessary to ensure our ordering is paramount.
Rails 3.2 compatibility is maintained via the
exceptmethod insteadof
unscope.Fixes #263