Update patch set 5

Patch Set 5: Code-Review-1

(4 comments)

Thanks a lot, I understand now what's weird in that recursion (your algorithm leads to correct results but is mixing two common patterns I think). I have written a long inline comment, I hope it's not too "overwhelming", but I suggest an easy fix at the end of it. Let me know your thoughts. It may well be that I got it wrong again, but I think now I understand better the intent.

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

16fa10433ca040b01f6216b9ef95ec97de54eb0e

@@ -0,0 +1,72 @@
+{
+  "comments": [
+    {
+      "key": {
+        "uuid": "47c7b38e_834e872c",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 5
+      },
+      "lineNbr": 589,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-25T13:19:46Z",
+      "side": 1,
+      "message": "I still think this should be called _get_descendants() because it\u0027s not returning only (direct) children, it\u0027s returning also grand-(grand-(...))children, correct?",
+      "revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "54b9c32a_7fd7eee3",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 5
+      },
+      "lineNbr": 625,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-25T13:19:46Z",
+      "side": 1,
+      "message": "If I got it right, I think it should be plural as it returns a several descendants",
+      "revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "8c22b177_2e6cffcb",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 5
+      },
+      "lineNbr": 626,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-25T13:19:46Z",
+      "side": 1,
+      "message": "So here too I think this should be plural as well",
+      "revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    },
+    {
+      "key": {
+        "uuid": "9f2da9d5_82c3d518",
+        "filename": "charms_ceph/utils.py",
+        "patchSetId": 5
+      },
+      "lineNbr": 645,
+      "author": {
+        "id": 31289
+      },
+      "writtenOn": "2021-06-25T13:19:46Z",
+      "side": 1,
+      "message": "So the weird thing about this entire recursion is that at each level we do two things:\n- we populate a \"global\" list called \"descendants\"\n- we return that global list.\n\nThis doesn\u0027t look like recursions I have seen before. Usually a recursion looks something like this:\n\n```\ndef recursive_func(parent):\n    if stop_condition:\n        return something\n    return aggregation of current level and recursive_func(parent.children)\n```\n\nDo you see what I mean? There is no global list that gets populated at each level.\n\nNow true, I have also seen recursions written with a global collection and no return value, then it looks like:\n\n```\nresult \u003d []\ndef recursive_func(parent):\n    if stop_condition:\n        return\n    result.append(something from current level)\n    recursive_func(parent.children)\n```\n\nBut in your recursion, you\u0027re mixing both patterns: the one based on aggregating returned values and the one where we populate a global collection.\n\nNow based on the example you have given in the docstring, I don\u0027t think this is needed. I think you can pick one of both patterns. What do you think?\n\nI think if you do just \"return\" on line 645 and transform line 647 in\n\n```\nget_descendant(parent, [parent[\"id\"]])\nreturn descendants\n```\n\nthen you get the same final results and it then follows pattern number 2 (the one populating a global collection). What do you think?",
+      "revId": "16fa10433ca040b01f6216b9ef95ec97de54eb0e",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543",
+      "unresolved": true
+    }
+  ]
+}