search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Be more surgical about unscoping #265

Closed brendon closed 7 years ago

brendon commented 7 years ago

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.

Fixes https://github.com/swanandp/acts_as_list/issues/263

brendon commented 7 years ago

Bummer. Looks like Rails 3.2 isn't working yet. I'll keep going.

brendon commented 7 years ago

Sadly, in Rails 3.2 except(:where).where() doesn't work as expected and still leaves in the default_scope.

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 unscoped do since this is just a refinement of the functionality.

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.

swanandp commented 7 years ago

Proposal: For all the 3.2 and backwards compatibility ~hacks~ fixes, we should add a mixin, and override there. This will reduce branching, and put all the code in one place.

brendon commented 7 years ago

RE the Mixin, that sounds like a good idea. I'll have a go at that now.

brendon commented 7 years ago

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?

swanandp commented 7 years ago

On Tue, 4 Apr 2017 at 3:15 AM, Brendon Muir notifications@github.com 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 https://github.com/swanandp/acts_as_list/pull/265#issuecomment-291283558, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFjGFopdyZjVoPePqxVgBNH9GFULVSsks5rsWiIgaJpZM4MuI4Q .