search

collectiveidea / awesome_nested_set

An awesome replacement for acts_as_nested_set and better_nested_set.
MIT License
2.39k stars 491 forks source link

Teach `#move_to_child_with_index` to accept `:root` as a target #481

Closed botandrose closed 6 months ago

botandrose commented 6 months ago

Overview:

This PR enhances the #move_to_child_with_index method to accept :root as a valid parent:

categories(:child_1).move_to_child_with_index(:root, 1)

Motivation:

The motivation was pairing ANS with a JavaScript drag-and-drop tree reordering library, and needing to persist the moves made to the database. The #move_to_child_with_index API was perfect for this, except it couldn't handle moves made to the top-level. This has been noted before in https://github.com/collectiveidea/awesome_nested_set/issues/446

Questions:

Does this look like a reasonable addition to ANS? Is this the right API design? Would passing nil instead of :root be more consistent with the rest of the API? Or perhaps it should be a new method entirely, like https://github.com/collectiveidea/awesome_nested_set/pull/447 ?

Future work:

If this is the right API design, I'd like to refactor out the duplication in #move_to_child_with_index. Also, I'd like to add a few more tests.

What do you think?

botandrose commented 6 months ago

Looks like CI is broken on master, cuz rails-main depends on Ruby 3.x now. Fixed in https://github.com/collectiveidea/awesome_nested_set/pull/482

botandrose commented 6 months ago

@parndt ok, green after rebasing on master

parndt commented 6 months ago

@botandrose given we already have this code move_to self, :root which special cases the term :root I'm happy with this API design too, so feel free to refactor this please in this PR if you're willing to put in the time 😄

botandrose commented 6 months ago

@parndt I started the refactoring today, and its becoming clear to me that in order to remove the duplication, I'll need to introduce two more API additions:

  1. Add ANS::Model::Relatable#roots, for getting a relation of the root nodes of the tree of the current node. This seems like an obvious hole in the API, and there is a class-method equivalent, so I don't expect this to be particularly controversial.
  2. Teach ANS::Model::Movable#move_to_child_of(node) to also accept :root as an argument. Given this PR's validity, this seems like it should also work too, and if it does, #move_to_child_with_index can use it internally to remove a lot of duplication.

My question for you is whether or not I should open separate PRs for these two items to evaluate and discuss one-by-one, or if you'd prefer this whole endeavour to live in this one PR as a coherent package?

parndt commented 6 months ago

@botandrose happy for it to be one change if you are 😄 then it provides the whole context for anyone looking later. IF you'd prefer to do it separately, I'm fine with that too.

botandrose commented 6 months ago

@parndt Okay, I'm pretty satisfied with this, now. Do you think it needs anything more before its good to merge?