pazz / urwidtrees

tree widgets for urwid
GNU General Public License v3.0
52 stars 18 forks source link

New Tree methods: leafs(pos) and parents(pos) #33

Closed rndusr closed 7 years ago

rndusr commented 7 years ago

leafs(pos) yields all leaf nodes below pos recursively. parents(pos) does the same for non-leaf nodes.

pazz commented 7 years ago

is there a reason why you use yield and instead of simple return statements? I kind of see the point for the leafs method, but what's your usecase for Tree.parents?

rndusr commented 7 years ago

is there a reason why you use yield and instead of simple return statements?

If a sequence is needed, it's easy to put the leafs/parents into a tuple or list. But if an iterator is enough (as in for item in tree.leafs(pos)) generators are more efficient.

I kind of see the point for the leafs method, but what's your usecase for Tree.parents?

I currently don't have a usecase, but I thought it would be weird if there was only an iterator for leafs and not one for parents also. It's only six lines of code and the implementation is very similar to leafs. Shouldn't be much of a burden to maintain.

But I put each in a separate commit so you can only pick leafs if you think parents is useless.

rndusr commented 7 years ago

Oh, but I just noticed the yield from, which I think is python3 only. If you support python2, I'll fix that.

pazz commented 7 years ago

yes, we definitely need python2 support (I use this lib in https://github.com/pazz/alot which officially only supports python2 for now).

I'd rather see return statements here as well, to be consistent with the rest of the lib. Also, after refactoring (use return, and rename to correct spelling leaves) I'd push the first patch but I'm no huge fan of the second. @lucc: your opinion?

rndusr commented 7 years ago

yes, we definitely need python2 support

OK.

I'd rather see return statements here as well, to be consistent with the rest of the lib.

I don't really mind, but Tree.positions is also a generator?

I also have a side question: What does "DFO" mean? I don't understand what positions does from reading the docstring.

And another side question: Is there a reason positions returns a generator function instead of just being a generator itself? Everything should work identically if you remove the first line ("def Posgen...") and the last line ("return Posgen..."):

def positions(self, reverse=False):
    if reverse:
        lastrootsib = self.last_sibling_position(self.root)
        current = self.last_decendant(lastrootsib)
        while current is not None:
            yield current
            current = self.prev_position(current)
    else:
        current = self.root
        while current is not None:
            yield current
            current = self.next_position(current)
rndusr commented 7 years ago

I removed parents, fixed the spelling and leaves now returns a list.

lucc commented 7 years ago

@pazz I am not sure what I sould review here as I have no understanding of the internals up till now. So I can not evaluate if this is a neccessary addition to the api or not.

pazz commented 7 years ago

thanks for having a look at this @lucc. It seems that this would be safe but I don't see the big benefit at this point. I'll close this PR. It can still be found through the search if s/o needs it later.