Fix the handling of EtcdResult children

[jerrykan]

An EtcdResult object would return itself when it had no children. This
could lead to infinite loops when attempting to parse down into paths.
It would be best to just return an empty list.

There was also a bug where if a EtcdResult object did have children, the
grand children would be returned instead of the direct children. This
has now been fixed.

[lavagetto]

The EtcdResult.children just returns the leafs of the tree (so, keys with values and not directories), and that was its intended use. Think of it as a "find . -type f" for etcd. The reason for that is the most common use of a recursive=true call to an etcd tree is to retreive the values of the leafs. I do get that "children" may be a misleading name, we should document that better.

Also, since the actual EtcdResult.children already looks completely down a path for you, there is no reason to do strange loops.

Just to be clear: imagine you have a 5-level nested config tree under /v2/keys/myapp. If I want to fetch all the config keys in the actual version of EtcdResult this would be:

c = etcd.Client()
for config_key in c.read('/myapp', recursive=True).children:
    print("%s: %s" % (config_key.key, config_key.value))

With your patch, this would require nested loops that are exactly the pain I wanted to avoid to our users.

This is not a bug, but a feature - sorry but I waited a long time to be able to say this :).

If you think having a function returning the immediate siblings would be useful, we may incorporate your patch as a different property (maybe 'siblings'? I'm not a native english speaker so advice is welcome). Can you present me a use-case where having the direct siblings only makes sense?

[jerrykan]

OK that makes sense. I was trying to figure out how the values being returned could have been so obviously wrong ;)

The name 'children' heavily implies that EtcdResult is using some sort of tree structure (and the internal dict reflects this), so I think lots of people would jump to the same conclusion as I did about the use for the function. Instead of trying to work against these assumptions by documenting that the method "is not what you think" (and hoping people read the documentation) I would suggest changing the method name if at all possible. By changing the method name to reflect what it is actually returning it would greatly help in avoiding any confusion.

Feel free to close the issue if you disagree.

[lavagetto]

@jerrykan I agree with you - do you have any suggestion for an alternative name? I'll still keep "children" around as a deprecated alias though.

[jerrykan]

Yes, keeping a deprecated alias for 'children' makes sense. The only alternative that comes to mind at the moment is 'values', but that may also be potentially misleading given python's list.key()/list.values() functions. Alternatively 'value_nodes', but that isn't particularly inspiring either.

I'll have a think and see if anything else comes to mind... unless you come up with something in the meantime.

[lavagetto]

@jplana any take on this? I'm utterly non-creative with names :)

[sigmunau]

I'd say either leaves or values() (similar to dict). Also, I'd like a way to get directories as well.

[lavagetto]

@sigmunau meaning you'd like to get every subnode, including dirs. That's fair, it's not super-complicated as well.

I think leaves() may be the better choice, but I'm not a great fan of it.

For now, I'll close this PR and create an issue

[jplana]

@lavagetto I'm not very creative with names either :-D, but "leaves" might be closer to graph theory language.

[lavagetto]

I'm closing the PR as I've opened an issue on that; I'm also going to open a feature branch soon