Hobo Hobo → Hobo 1.0X

Comments and changes to this ticket

• 

  Bryan Larsen November 24th, 2010 @ 08:32 PM

  Here's a breaking test for the condition.

  • scopes.rdoctest 10.2 KB
• 

  Bryan Larsen November 24th, 2010 @ 09:12 PM

  and here's a quick hack patch, which works against all the rubies & rails in our system tests.

  • 0001-873-simple-hack-fix-for-chained-scopes.patch 967 Bytes
• 

  Bryan Larsen November 27th, 2010 @ 04:07 AM

  (from [9bb733f7a1e619b8e5fbcc3679eab49c0dc08734]) [#839] [#873] fix chained scopes, as well as fix the original patch for bug #839 so that it works with :include and 1.9.2 https://github.com/tablatom/hobo/commit/9bb733f7a1e619b8e5fbcc3679e...

• 

  Matt Jones November 27th, 2010 @ 07:19 PM

  Here's a slightly clearer patch (IMHO) that implements automatic scopes differently; essentially, the scopes hash on the model class is "spring-loaded" to create scopes when missing keys are encountered. There's some extra work needed to make include? work correctly, which is mostly used by Rails 2.3.x (Rails 3 replaces calls to scopes.include?(name) to just scopes[name]).

  This allows us to leave proxy_responds_to? and method_missing completely alone on the Scope class; it also means that any other extension / plugin that pokes around in the scopes array will trigger consistent behavior.

  I'd appreciate some further testing on this - I'd do it, but all my production apps are still back on Rails 2.3.5 which doesn't experience this problem. I did add some doctests, but I don't think the coverage is 100%.

  Patch is against 1-0-stable, but it should be easy to port over to 1.3.

  • spring_loaded.patch 3.6 KB
• 

  Domizio Demichelis November 27th, 2010 @ 07:47 PM

  Patch is against 1-0-stable, but it should be easy to port over to 1.3.

  Matt,

  There is no need to patch the 1.3 because I've managed to avoid the AR patches needed for scope completely. I refactoried the automatic scopes, and now they should work without any patch.

  Instead the :scope key for associations, still use AR patches, although completely different from previous versions.

  About scopes, I deprecated the use of :include in favour of :including, which is indeed a really bad naming choice IMHO, specially for a method that is created on the fly. It clashes with Module#include when called on an ActiveRecord::Relation after a scope chain, if you don't call it on the class itself first. In this case we don't need to monkey patch AR just for using :include. I think that just using a wiser name is a better solution.

• 

  Matt Jones November 28th, 2010 @ 01:01 AM

  Heh - that's what I get for flipping through too many different source versions at once. :)

  Anyways, do we still need the 'including' scope at all? It appears that the Arel version ('includes') does exactly the same thing (and 'including' calls 'includes' directly). I noticed the 'limit' automatic scope doesn't need to exist anymore, so wouldn't the same logic apply to 'including'?

• 

  Domizio Demichelis November 28th, 2010 @ 01:12 AM

  You are absolutely right!

  To tell it with your own words "that's what I get for flipping through too many different" refactories. :-) I completely missed that! It doesn't make any sense using :including just to call :includes. Thank you.

• 

  Bryan Larsen November 28th, 2010 @ 06:00 PM

  We can't change the API for 1.0, so we're stuck with either Matt's patch or mine. However, we can for 1.1 if we have a good reason. Would it be possible to rename "include" to "includes" for 1.1, and use native AREL for 1.3? Is our usage compatible with AREL's usage?

• 

  Domizio Demichelis November 28th, 2010 @ 07:52 PM

  Would it be possible to rename "include" to "includes" for 1.1, and use native AREL for 1.3? Is our usage compatible with AREL's usage?

  I have deprecated :include in 1.3 in favour of :includes: users have just to use the latter and the rails / Arel get called because :includes is already a defined model's method, but I am not so sure you really need to use :includes in 1.1.

  Your latest addition to scope doctest was failing with 1.3, and the failure revealed the Module#include clash, but I guess that if you added it, you probably tested it against 1.1, and if it passes that means that for some reason it doesn't clash so we don't need to change anything...

  or you are meaning something completely different, and I am answering obvious and useless things as usual :-)

• 

  Domizio Demichelis November 28th, 2010 @ 08:15 PM

  aghhhh... I think I got it all wrong :-)

  You (probably) mean that you could add :incudes in 1.1 so that the transition to 1.3 will be smoother, and you want to know whether the method will accept the same arguments as 1.3 does. Is that right?

  Well, yes, the rails/AR/Arel :incudes method accepts an association symbol. I had just to change the method name in your test in order to make it pass.

  Usually I am not so dumb understanding others, but I noticed that for some reason I have that problem with your posts. :-)

• 

  Bryan Larsen December 1st, 2010 @ 07:11 PM

  Matt, there are a few issues with your patch:

  • on Rails 2.3.5 it triggers bug #839 (wrong argument type Symbol (expected Module) with the include scope)
  • with Ruby 1.9.2 it triggers "RuntimeError: implicit argument passing of super from method defined by define_method() is not supported. Specify all arguments explicitly."
  • the use of "tap" means that we lose ruby 1.8.6 compatibility. I know we have people using 1.8.6, although I'm not sure whether they're on 1.0 or on 0.8.x.

  The latter two issues are easy to fix.

  The former took me quite a while to figure out why it breaks. In 2.3.5, "scoped" is defined as a scope in self.included, so the scopes hash gets created before your new scopes method gets defined.

  I uglified up your patch and attached it.

  • named_scope_extensions.rb 911 Bytes
• 

  Bryan Larsen December 9th, 2010 @ 03:12 PM

  As Gert pointed out, we also need to make sure that we don't kill non-hobo_model models.

  • named_scope_extensions.rb 1.1 KB
• 

  Bryan Larsen December 9th, 2010 @ 03:13 PM

  oops, forgot a piece

  • named_scope_extensions.rb 1.1 KB
• 

  Bryan Larsen January 15th, 2011 @ 03:17 PM

  (from [863daab7878bfb4faf7a7deca2dece795562454d]) [#873] more work to ensure we don't break hobo models https://github.com/tablatom/hobo/commit/863daab7878bfb4faf7a7deca2d...

• 

  Bryan Larsen January 17th, 2011 @ 10:42 PM

  (from [e0330b3134a1713cddbb79a1f2c8e8008ae5b9ef]) [#873] Matt's additional scope tests from his spring_loaded.patch https://github.com/tablatom/hobo/commit/e0330b3134a1713cddbb79a1f2c...

• 

  Bryan Larsen January 17th, 2011 @ 10:42 PM

  (from [b46beb9d93ec096e7fbdd3fa1077048671c9e07a]) [#873] [#839] fix chained scopes and the :include scope. Credit: Matt Jones
  https://github.com/tablatom/hobo/commit/b46beb9d93ec096e7fbdd3fa107...

• 

  Bryan Larsen January 17th, 2011 @ 10:42 PM

  (from [ee3daaa3187bab5445e696cbd9ae41e3ee0d3a87]) [#873] [#839] add "includes" automatic scope which is identical to "include", deprecating "include" https://github.com/tablatom/hobo/commit/ee3daaa3187bab5445e696cbd9a...

• 

  Bryan Larsen January 17th, 2011 @ 10:43 PM

  (from [29ee9818ba3254b821b458115cf1a1a068816a95]) [#873] more work to ensure we don't break hobo models https://github.com/tablatom/hobo/commit/29ee9818ba3254b821b458115cf...