Rails core - Aug 2006 - [PATCH] Giving acts_as_nested_set some love and moving nodes around in a tree

I''ve added a new feature to acts_as_nested_set, namely the ability to  
move a node within a tree.

It is accomplished with calls ActiveRecord::Base.update_all, works  
fine on a whole subtree, and works by opening a gap for the  
destination, moving, and then closing the gap at the origin.

But I also did something else.

The code used "foo( bar )" instead of "foo(bar)", it had the
expected
and actual results reversed in "assert_equal" in the tests, and a few
other things. I took the liberty of fixing all of these while I was  
in there, anyway.

What''s the policy on changing existing code to adhere to coding  
standards? Is that a good thing or a bad thing to do? Is it better to  
leave it, to keep the patch simpler?

I''m personally in favor of making code adhere to the standards,  
because it makes it easier to read, and because it increases the  
chance that people will adhere to them going forward. And looking at  
code that''s half-adherent-half-not is even worse than pure non- 
adherence.

The code could use even more cleaning -- it feels a bit fragile, for  
example it does updates both on the object directly and using  
update_all, which means it has to reload the objects time and again.  
Also, if you manually change the parent_id of an object, the nested  
sets aren''t updated, so you have to be fairly careful with how you  
interact with your objects.

My immediate needs are met with the current incarnation, though, so  
I''m looking to gauge interest and opinions before I progress further.

Patch attach! ...ed!

/Lars
  

_______________________________________________
Rails-core mailing list
Rails-core@lists.rubyonrails.org
http://lists.rubyonrails.org/mailman/listinfo/rails-core

On Aug 2, 2006, at 3:28 AM, Lars Pind wrote:
> I''ve added a new feature to acts_as_nested_set, namely the ability
> to move a node within a tree.
>
> It is accomplished with calls ActiveRecord::Base.update_all, works  
> fine on a whole subtree, and works by opening a gap for the  
> destination, moving, and then closing the gap at the origin.
>
<snip>
> What''s the policy on changing existing code to adhere to coding  
> standards? Is that a good thing or a bad thing to do? Is it better  
> to leave it, to keep the patch simpler?
>
> I''m personally in favor of making code adhere to the standards,  
> because it makes it easier to read, and because it increases the  
> chance that people will adhere to them going forward. And looking  
> at code that''s half-adherent-half-not is even worse than pure non-
> adherence.

Is nested_set something that should be moved to, say, a plugin and a  
rubyforge project?  Particularly given the few people using it, and  
the ''fragility'' of the code..



________
courtenay
http://blog.caboo.se @o O@

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Aug 2, 2006, at 6:37 AM, court3nay wrote:
> On Aug 2, 2006, at 3:28 AM, Lars Pind wrote:
>
>> I''ve added a new feature to acts_as_nested_set, namely the
ability
>> to move a node within a tree.
>>
>> It is accomplished with calls ActiveRecord::Base.update_all, works  
>> fine on a whole subtree, and works by opening a gap for the  
>> destination, moving, and then closing the gap at the origin.
>>
> <snip>
>> What''s the policy on changing existing code to adhere to
coding
>> standards? Is that a good thing or a bad thing to do? Is it better  
>> to leave it, to keep the patch simpler?
>>
>> I''m personally in favor of making code adhere to the
standards,
>> because it makes it easier to read, and because it increases the  
>> chance that people will adhere to them going forward. And looking  
>> at code that''s half-adherent-half-not is even worse than pure
non-
>> adherence.
>
>
> Is nested_set something that should be moved to, say, a plugin and  
> a rubyforge project?  Particularly given the few people using it,  
> and the ''fragility'' of the code..
>
>
+1. Not to mention smaller projects can be more quickly updated and  
it unloads the patch cycle from core...
>
> ________
> courtenay
> http://blog.caboo.se @o O@
> _______________________________________________
> Rails-core mailing list
> Rails-core@lists.rubyonrails.org
> http://lists.rubyonrails.org/mailman/listinfo/rails-core
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (Darwin)

iD8DBQFE0LWaqvuZB2zXNU0RArhEAJ9d2dOiekTSANx63Wop5R/At2Kc+wCgs8+g
0ULLkizHtGDCJwkCymLlC6o=EoYB
-----END PGP SIGNATURE-----

On 2-aug-2006, at 12:37, court3nay wrote:
>  Particularly given the few people using it, and the
''fragility'' of
> the code..
I don''t think it''s really "few".
-- 
Julian ''Julik'' Tarkhanov
please send all personal mail to
me at julik.nl

On Aug 2, 2006, at 9:43 AM, Julian ''Julik'' Tarkhanov wrote:
>>  Particularly given the few people using it, and the
''fragility''
>> of the code..
>
> I don''t think it''s really "few".

I still can''t get it to work for my simple examples in the book, and  
I''m not documenting it in the second edition because of that.
I''ve
tried to find folks to look at the problems I''m seeing, but to no  
avail, so as far as it seems from the outside (to me at least) this  
is unworking and unmaintained code.


Dave

> >>  Particularly given the few people using it, and the
''fragility''
> >> of the code..
> >
> > I don''t think it''s really "few".
>
>
> I still can''t get it to work for my simple examples in the book,
and
> I''m not documenting it in the second edition because of that.
I''ve
> tried to find folks to look at the problems I''m seeing, but to no
> avail, so as far as it seems from the outside (to me at least) this
> is unworking and unmaintained code.
act_as_nested_set is definitely on the growing list of things that''ll
be split into plugins by 1.3. Others on that list includes all the
Ajax macros (in_place_editor etc), pagination, and possibly
acts_as_list/acts_as_tree.
-- 
David Heinemeier Hansson
http://www.loudthinking.com -- Broadcasting Brain
http://www.basecamphq.com   -- Online project management
http://www.backpackit.com   -- Personal information manager
http://www.rubyonrails.com  -- Web-application framework

Hi,

I''ve been working on it already, and after a patch I submitted a while 
ago, I made it a plugin:

http://opensource.symetrie.com/trac/better_nested_set/

I''ll update the tests and show how I use it for recursive menus and 
categories.
I''m open to any improvements, let''s not duplicate efforts ;-)

Le 2 août 06, à 12:28, Lars Pind a écrit :
> I''ve added a new feature to acts_as_nested_set, namely the ability
to
> move a node within a tree.
Jean-Christophe Michel
--
Symétrie, édition de musique et services multimédia
30 rue Jean-Baptiste Say
69001 LYON (FRANCE)
tél +33 (0)478 29 52 14
fax +33 (0)478 30 01 11
web www.symetrie.com

Dave Thomas wrote:
> I still can''t get it to work for my simple examples in the book,
and
> I''m not documenting it in the second edition because of that.
I''ve
> tried to find folks to look at the problems I''m seeing, but to no 
> avail, so as far as it seems from the outside (to me at least) this is 
> unworking and unmaintained code.
I have implemented a replacement for acts_as_nested_set as part of an 
ongoing consulting gig. I''m confident it''ll make your tests
pass. It
extends the current implementation in several ways: subtrees can be 
removed from one tree and added to another tree and trees can be sorted 
for display.

I''m currently in the process of obtaining the permission to open source
the code. If all goes well, I''ll offer it as a plugin.

-- stefan

-- 
Rails performance tuning: http://railsexpress.de/blog
Subscription: http://railsexpress.de/blog/xml/rss20/feed.xml