Update patch set 4

Patch Set 4: Code-Review-1

(4 comments)

Thanks a lot. I stopped reviewing at "_get_children()" because I couldn't understand it fully. I have tried to capture my difficulties in inline comments. I suspect the algorithm may be correct but the wrong words are used (sometime child instead of children, sometimes children instead of descendants, etc.) and this makes it hard to follow. Thanks!

Patch-set: 4
Reviewer: Gerrit User 31289 <31289@4a232e18-c5a9-48ee-94c0-e04e7cca6543>
Label: Code-Review=-1

9b34af1aea048f72cb1bd840aec19a5068a83009

@@ -0,0 +1,72 @@
+{
+  "comments": [
+    {
+      "key": {
+        "uuid": "34b08069_38a4e1fb",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 4
+      },
+      "lineNbr": 590,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-23T13:27:38Z",
+      "side": 1,
+      "message": "I think you mean \"get all the children of a given parent\"?",
+      "revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "82835ef7_83d6f74d",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 4
+      },
+      "lineNbr": 593,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-23T13:27:38Z",
+      "side": 1,
+      "message": "this is confusing: this is called \"get_child\" but in fact returns several children. Should this be called \"get_children\" instead?",
+      "revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "6a1eec33_e9ad3bb2",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 4
+      },
+      "lineNbr": 595,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-23T13:27:38Z",
+      "side": 1,
+      "message": "If I understand correctly this means \"if this is a leaf OR if this node has the wanted type\". That means that _get_children(..., node_type) will return all leaves of the tree AND the non-leaves having the type node_type. I think this is not what one would instinctively expect from the signature of this function and should at least be explained in the docstring.\n\nAlso, is this really the intended behavior? I\u0027m not sure how this makes sense in the big picture, but that\u0027s probably because I\u0027m not a ceph expert.",
+      "revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "8f44eb7b_69765dbf",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 4
+      },
+      "lineNbr": 604,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-23T13:27:38Z",
+      "side": 1,
+      "message": "are we now merging child information and parent information inside a Set? I\u0027m lost, sorry, because I would have expected the parents to have been dealt with in the previous level of the recursion.\n\nIt may be the best algorithm here and I\u0027m failing to understand it. It think it would be useful to have:\n- more elaborated docstrings\n- clearer variable names or comments: all children and their children recursively are called **descendants** (please use that word if that\u0027s what you mean). All parents and their parents recursively are called ancestors (please use that work if that\u0027s what you mean)\n- An example of the whole structure.\n\nThen this will be easier to resonate about the algorithm and extend it some day. Thanks a lot!",
+      "revId": "9b34af1aea048f72cb1bd840aec19a5068a83009",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    }
+  ]
+}