move_node() allows you to create loops

[mgedmin]

Example:

>>> from treelib import Tree
>>> tree = Tree()
>>> tree.create_node('a', 'a')
>>> tree.create_node('b', 'b', parent='a')
>>> tree.show()
a
└── b
>>> tree.move_node('a', 'b')
>>> tree.show()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/mg/src/tilaajavastuu/bolagsfakta/server/env/lib/python3.5/site-packages/treelib/tree.py", line 626, in show
    key, reverse, line_type, data_property, func=write)
  File "/home/mg/src/tilaajavastuu/bolagsfakta/server/env/lib/python3.5/site-packages/treelib/tree.py", line 205, in __print_backend
    filter, key, reverse, line_type, data_property, func, iflast)
...
  File "/home/mg/src/tilaajavastuu/bolagsfakta/server/env/lib/python3.5/site-packages/treelib/tree.py", line 198, in <listcomp>
    queue = [self[i] for i in self[nid].fpointer if filter(self[i])]
RecursionError: maximum recursion depth exceeded

[UmanShahzad]

See PR #68 for my attempt at a solution to this (and to any situation where you try to call move_node() between a parent/child; this caused them to simply disappear into thin air)

[mgedmin]

That still leaves a bug when you try to move a node under one of its grandchildren. You need to check if the node you're moving appears anywhere in the destination's ancestor list, and disallow that move.

[UmanShahzad]

I think the more glaring problem is that move_node()'s intentions seem really broad, and we're having to one by one disable what it (maybe?) should do. Because, for example, how do we know whether the original author would be okay with disabling movement to the destination if it is a grandchild? This is something that had to be said beforehand.

I think this function can use some extension: add an optional mode parameter which defaults to a 'smart' move strategy, and some other modes like force or safe which will have well pre-defined strategies in case the user wants them.

Either that, or list out case by case what is permitted by this function and what isn't. If too little is actually permitted, the function should be deprecated and a different set of functions to handle the few permitted cases should be added.

I'll wait on the repository maintainer's opinion on this.

[caesar0301]

@mgedmin thanks for your issuing and @UmanShahzad 's solution, honestly partially. As you said, we will lose generality if merely checking parent/children or grandchildren. A straightforward and generic solution may be making a loop validation before taking actions. Otherwise, a LoopException raised.

[UmanShahzad]

@caesar0301 I will see if I can do that soon, and will update the PR

[UmanShahzad]

@caesar0301 Please see PR #68 now, thank you!