Betternestedset talk - Dec 2006 - Questions, RC for 0.1?

Hi JCM et al.,

I''ve been committing a bunch of changes over the last few days, and
we''re down to one ticket on trac now, so I think we should start
considering the current code as a release candidate for 0.1.

I''ve come across some things I wanted to ask about, though.

First of all, I think that #move_to_child_of should insert the new
child to the right of existing children. Here''s why:
- consistent with the creation of new records (to the right of existing ones)
- consistent with #add_child
- consistent with default ordering for things like threaded posts,
where new items are added after existing ones
- if the parent has a large number of children relative to the size of
the tree, performance will be better, because fewer rows need to be
updated

What do you think? I know this will be a compatibility issue, so if we
are going to do it  perhaps we should wait until after 0.1.

The second thing I want to talk about would also cause compatibility
problems. ;)  It concerns #children_count. The present name of the
method is highly misleading, because it actually counts _all_ children
of the node, so presently node.children.size != node.children_count.
If we are going to change it to #all_children_count, I would favor
changing it to the much more natural #count_all_children instead. (I
would also change #leaves_count to #count_leaves). Thoughts?

The third issue concerns concurrency problems. Prior to revision 25,
the following code would corrupt the left/right indexes:

    c1, c2, c3 = Category.create, Category.create, Category.create
    c1.move_to_right_of(c3)
    c2.save  # c2 is stale, and overwrites the correct lft/rgt values

My solution was to override the #update method in ActiveRecord to
ignore the lft/rgt columns (the code is at the bottom of
better_nested_set.rb). I don''t know if this is a good way to do
things, so I''m hoping that someone else can take a look at the code
and see if it is sane.


Cheers,
Krishna

Hi Krishna,

Le 13 d?c. 06, ? 02:03, Krishna Dole a ?crit :
> I''ve been committing a bunch of changes over the last few days,
and
> we''re down to one ticket on trac now, so I think we should start
> considering the current code as a release candidate for 0.1.
Sure, you''ve done a considerable amount of work. Many thanks !
> First of all, I think that #move_to_child_of should insert the new
> child to the right of existing children. Here''s why:
> - consistent with the creation of new records (to the right of 
> existing ones)
> - consistent with #add_child
> - consistent with default ordering for things like threaded posts,
> where new items are added after existing ones
> - if the parent has a large number of children relative to the size of
> the tree, performance will be better, because fewer rows need to be
> updated
>
> What do you think? I know this will be a compatibility issue, so if we
> are going to do it  perhaps we should wait until after 0.1.
why not. Let''s change this before 0.1.
> The second thing I want to talk about would also cause compatibility
> problems. ;)  It concerns #children_count. The present name of the
> method is highly misleading, because it actually counts _all_ children
> of the node, so presently node.children.size != node.children_count.
> If we are going to change it to #all_children_count, I would favor
> changing it to the much more natural #count_all_children instead. (I
> would also change #leaves_count to #count_leaves). Thoughts?
I copied behaviour as much as possible from acts_as_tree. I agree the 
meaning is not clear.
If we choose to break acts_as_tree similarity, we can rename it.
Or do we need to open a better_acts_as_tree project ? ;-)
> The third issue concerns concurrency problems. Prior to revision 25,
> the following code would corrupt the left/right indexes:
>
>     c1, c2, c3 = Category.create, Category.create, Category.create
>     c1.move_to_right_of(c3)
>     c2.save  # c2 is stale, and overwrites the correct lft/rgt values
>
> My solution was to override the #update method in ActiveRecord to
> ignore the lft/rgt columns (the code is at the bottom of
> better_nested_set.rb). I don''t know if this is a good way to do
> things, so I''m hoping that someone else can take a look at the
code
> and see if it is sane.
Don''t you think we''ll solve the problem by defining
   attr_reader left_column_name
   or maybe, define #{left_column}=() {} method
so none of left, right and parent could be set from code (only changed 
from sql)
DId you try this already ?

Jean-Christophe Michel
--
symetrie.com

Better Nested Set for rails:
http://opensource.symetrie.com/trac/better_nested_set

Hi Jean-Christophe,

Sorry to be so slow in responding.
> why not. Let''s change this before 0.1.
OK, I''ll make move_to_child_of insert on the right.
> I copied behaviour as much as possible from acts_as_tree.
Ah, I see that now. I suppose remaining consistent with the other
counter cache methods in rails is good. You are OK with changing it to
all_children_count, correct? (since the current name is inconsistent
with both acts_as_tree and our other methods)
> Don''t you think we''ll solve the problem by defining
>    attr_reader left_column_name
You mean so that when ActiveRecord goes to update, it gets nil for the
column values? That would still be a problem, because it would then
set the values to NULL.
>    or maybe, define #{left_column}=() {} method
> so none of left, right and parent could be set from code (only changed
> from sql)
We already prevent external assignment of lft/rgt, and ActiveRecord
needs to set those attributes when a record if fetched. The problem is
not that the values are being changed in the object, but that they are
being changed in the database when a separate object is moved/deleted,
and then if the record is saved, the (now incorrect) values get
written back to the database. ActiveRecord does not appear to have any
predefined way to exclude particular columns from the update
statement, hence my hack.

Please let me know if I''ve failed to see how the above might work.

I''m excited about how things are shaping up.

Cheers,
Krishna


>
> Jean-Christophe Michel
> --
> symetrie.com
>
> Better Nested Set for rails:
> http://opensource.symetrie.com/trac/better_nested_set
>
> _______________________________________________
> Betternestedset-talk mailing list
> Betternestedset-talk at rubyforge.org
> http://rubyforge.org/mailman/listinfo/betternestedset-talk
>