)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"319257d8fd90c64aa3a47ce83b399e41446c043a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"b547c465_fa55a108","updated":"2023-01-14 19:29:54.000000000","message":"A few thoughts inside","commit_id":"98fde23c51231526762e3305054a9f49b298f973"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"e536a7a5aaddbfcd374282ac27d0bf8127a9cf35","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":14,"id":"646fb816_f856431c","updated":"2023-01-28 11:24:29.000000000","message":"discussion in the code seems legit, but is not worth of holding the series for finally cutting R2.0. So +2 from me","commit_id":"16a8a9e5d4444278ca5c81be091de55e30f4ad81"}],"plugins/modules/identity_domain.py":[{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"319257d8fd90c64aa3a47ce83b399e41446c043a","unresolved":true,"context_lines":[{"line_number":98,"context_line":"    )"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    def run(self):"},{"line_number":101,"context_line":"        crud_functions \u003d StateMachine.default_crud_functions("},{"line_number":102,"context_line":"            self.conn, \u0027identity\u0027, \u0027domain\u0027)"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"        sm \u003d StateMachine(connection\u003dself.conn,"},{"line_number":105,"context_line":"                          crud_functions\u003dcrud_functions,"}],"source_content_type":"text/x-python","patch_set":3,"id":"b74885ed_8bae4b35","line":102,"range":{"start_line":101,"start_character":23,"end_line":102,"end_character":44},"updated":"2023-01-14 19:29:54.000000000","message":"Why to make a separate call for it? Maybe worth to pass (\u0027identity\u0027, \u0027domain\u0027) to \"StateMachine()\" call and get default_crud_functions in there.","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":32962,"name":"Jakob Meng","email":"code@jakobmeng.de","username":"jakobmeng@web.de"},"change_message_id":"65a9f41955bc1364feb17065b9a299e63775ddfc","unresolved":false,"context_lines":[{"line_number":98,"context_line":"    )"},{"line_number":99,"context_line":""},{"line_number":100,"context_line":"    def run(self):"},{"line_number":101,"context_line":"        crud_functions \u003d StateMachine.default_crud_functions("},{"line_number":102,"context_line":"            self.conn, \u0027identity\u0027, \u0027domain\u0027)"},{"line_number":103,"context_line":""},{"line_number":104,"context_line":"        sm \u003d StateMachine(connection\u003dself.conn,"},{"line_number":105,"context_line":"                          crud_functions\u003dcrud_functions,"}],"source_content_type":"text/x-python","patch_set":3,"id":"83537ca7_cf25004d","line":102,"range":{"start_line":101,"start_character":23,"end_line":102,"end_character":44},"in_reply_to":"b74885ed_8bae4b35","updated":"2023-01-16 13:08:34.000000000","message":"True! Done 😊","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"319257d8fd90c64aa3a47ce83b399e41446c043a","unresolved":true,"context_lines":[{"line_number":111,"context_line":"                      for k in [\u0027state\u0027, \u0027timeout\u0027]"},{"line_number":112,"context_line":"                      if self.params[k] is not None)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        kwargs[\u0027attributes\u0027] \u003d \\"},{"line_number":115,"context_line":"            dict((k, self.params[k])"},{"line_number":116,"context_line":"                 for k in [\u0027description\u0027, \u0027is_enabled\u0027, \u0027name\u0027]"},{"line_number":117,"context_line":"                 if self.params[k] is not None)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        domain, is_changed \u003d sm(check_mode\u003dself.ansible.check_mode,"},{"line_number":120,"context_line":"                                updateable_attributes\u003dNone,"}],"source_content_type":"text/x-python","patch_set":3,"id":"5afb8e2b_320f1a70","line":117,"range":{"start_line":114,"start_character":1,"end_line":117,"end_character":47},"updated":"2023-01-14 19:29:54.000000000","message":"Would be nice-to-have such a function in \"utils\", I see it a lot used - filtering out keywords with None.","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"a92b27fd3e1d79098002a95762ed0471d3bb9f19","unresolved":true,"context_lines":[{"line_number":111,"context_line":"                      for k in [\u0027state\u0027, \u0027timeout\u0027]"},{"line_number":112,"context_line":"                      if self.params[k] is not None)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        kwargs[\u0027attributes\u0027] \u003d \\"},{"line_number":115,"context_line":"            dict((k, self.params[k])"},{"line_number":116,"context_line":"                 for k in [\u0027description\u0027, \u0027is_enabled\u0027, \u0027name\u0027]"},{"line_number":117,"context_line":"                 if self.params[k] is not None)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        domain, is_changed \u003d sm(check_mode\u003dself.ansible.check_mode,"},{"line_number":120,"context_line":"                                updateable_attributes\u003dNone,"}],"source_content_type":"text/x-python","patch_set":3,"id":"80509037_dedcfebf","line":117,"range":{"start_line":114,"start_character":1,"end_line":117,"end_character":47},"in_reply_to":"2ac63959_4c58834d","updated":"2023-01-25 12:53:32.000000000","message":"If you name it as \"get_non_None_values\" no need to go and see what is going on, it\u0027s clear from its name. To prevent import you can actually to create a class method in StateMachine class 😊","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":32962,"name":"Jakob Meng","email":"code@jakobmeng.de","username":"jakobmeng@web.de"},"change_message_id":"65a9f41955bc1364feb17065b9a299e63775ddfc","unresolved":true,"context_lines":[{"line_number":111,"context_line":"                      for k in [\u0027state\u0027, \u0027timeout\u0027]"},{"line_number":112,"context_line":"                      if self.params[k] is not None)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        kwargs[\u0027attributes\u0027] \u003d \\"},{"line_number":115,"context_line":"            dict((k, self.params[k])"},{"line_number":116,"context_line":"                 for k in [\u0027description\u0027, \u0027is_enabled\u0027, \u0027name\u0027]"},{"line_number":117,"context_line":"                 if self.params[k] is not None)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        domain, is_changed \u003d sm(check_mode\u003dself.ansible.check_mode,"},{"line_number":120,"context_line":"                                updateable_attributes\u003dNone,"}],"source_content_type":"text/x-python","patch_set":3,"id":"2ac63959_4c58834d","line":117,"range":{"start_line":114,"start_character":1,"end_line":117,"end_character":47},"in_reply_to":"5afb8e2b_320f1a70","updated":"2023-01-16 13:08:34.000000000","message":"You would need an additional import and one would have to go one step further (into other file) to see what is going on. For only benefit of saving one line?","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":32962,"name":"Jakob Meng","email":"code@jakobmeng.de","username":"jakobmeng@web.de"},"change_message_id":"b36c34d4accfbc0f9c9d4af736da7f1513ca7e28","unresolved":true,"context_lines":[{"line_number":111,"context_line":"                      for k in [\u0027state\u0027, \u0027timeout\u0027]"},{"line_number":112,"context_line":"                      if self.params[k] is not None)"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":"        kwargs[\u0027attributes\u0027] \u003d \\"},{"line_number":115,"context_line":"            dict((k, self.params[k])"},{"line_number":116,"context_line":"                 for k in [\u0027description\u0027, \u0027is_enabled\u0027, \u0027name\u0027]"},{"line_number":117,"context_line":"                 if self.params[k] is not None)"},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"        domain, is_changed \u003d sm(check_mode\u003dself.ansible.check_mode,"},{"line_number":120,"context_line":"                                updateable_attributes\u003dNone,"}],"source_content_type":"text/x-python","patch_set":3,"id":"ef9f43a5_76f86498","line":117,"range":{"start_line":114,"start_character":1,"end_line":117,"end_character":47},"in_reply_to":"80509037_dedcfebf","updated":"2023-01-25 18:40:46.000000000","message":"Such a function would be very generic and not specific to StateMachine, so StateMachine would be the wrong place to put such a function. \n\nDefining such a function (anywhere) would trade simple Python code without redirections/function calls with... what benefit? Getting rid of a single for-loop? Not worth it imho.","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"319257d8fd90c64aa3a47ce83b399e41446c043a","unresolved":true,"context_lines":[{"line_number":122,"context_line":"                                wait\u003dFalse,"},{"line_number":123,"context_line":"                                **kwargs)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if domain is None:"},{"line_number":126,"context_line":"            self.exit_json(changed\u003dis_changed)"},{"line_number":127,"context_line":"        else:"},{"line_number":128,"context_line":"            self.exit_json(changed\u003dis_changed,"},{"line_number":129,"context_line":"                           domain\u003ddomain.to_dict(computed\u003dFalse))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"def main():"}],"source_content_type":"text/x-python","patch_set":3,"id":"23528829_3e442b0a","line":129,"range":{"start_line":125,"start_character":8,"end_line":129,"end_character":64},"updated":"2023-01-14 19:29:54.000000000","message":"We didn\u0027t really used it before, but you can try it: https://opendev.org/openstack/ansible-collections-openstack/src/branch/master/plugins/module_utils/openstack.py#L430\n\n  domain, self.results[\u0027changed\u0027] \u003d ....\n  if domain:\n    self.results[\u0027domain\u0027] \u003d domain","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":32962,"name":"Jakob Meng","email":"code@jakobmeng.de","username":"jakobmeng@web.de"},"change_message_id":"65a9f41955bc1364feb17065b9a299e63775ddfc","unresolved":true,"context_lines":[{"line_number":122,"context_line":"                                wait\u003dFalse,"},{"line_number":123,"context_line":"                                **kwargs)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if domain is None:"},{"line_number":126,"context_line":"            self.exit_json(changed\u003dis_changed)"},{"line_number":127,"context_line":"        else:"},{"line_number":128,"context_line":"            self.exit_json(changed\u003dis_changed,"},{"line_number":129,"context_line":"                           domain\u003ddomain.to_dict(computed\u003dFalse))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"def main():"}],"source_content_type":"text/x-python","patch_set":3,"id":"bb8abb6f_dc3821b4","line":129,"range":{"start_line":125,"start_character":8,"end_line":129,"end_character":64},"in_reply_to":"23528829_3e442b0a","updated":"2023-01-16 13:08:34.000000000","message":"This is hardly readable/comprehensible and not symmetric. The current way is more consistent with other modules.","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":32962,"name":"Jakob Meng","email":"code@jakobmeng.de","username":"jakobmeng@web.de"},"change_message_id":"b36c34d4accfbc0f9c9d4af736da7f1513ca7e28","unresolved":true,"context_lines":[{"line_number":122,"context_line":"                                wait\u003dFalse,"},{"line_number":123,"context_line":"                                **kwargs)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if domain is None:"},{"line_number":126,"context_line":"            self.exit_json(changed\u003dis_changed)"},{"line_number":127,"context_line":"        else:"},{"line_number":128,"context_line":"            self.exit_json(changed\u003dis_changed,"},{"line_number":129,"context_line":"                           domain\u003ddomain.to_dict(computed\u003dFalse))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"def main():"}],"source_content_type":"text/x-python","patch_set":3,"id":"ce515d1c_9291654f","line":129,"range":{"start_line":125,"start_character":8,"end_line":129,"end_character":64},"in_reply_to":"628deff9_167ad17d","updated":"2023-01-25 18:40:46.000000000","message":"Assigning one half of self.results in one line and the other half of self.results in another line breaks symmetry. Defining code branch of an if clause without the code branch for \u0027else\u0027 breaks symmetry. Combining both makes it harder to understand/grasp whats going on.\n\nIn other modules we tried to make it as easy as possible for our users to understand our code because they are not necessarily devs but operators who hit our bugs in our code. Consistency and symmetry helps here.","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"},{"author":{"_account_id":10969,"name":"Shnaidman Sagi (Sergey)","display_name":"Shnaidman Sagi","email":"sshnaidm@redhat.com","username":"sergsh"},"change_message_id":"a92b27fd3e1d79098002a95762ed0471d3bb9f19","unresolved":true,"context_lines":[{"line_number":122,"context_line":"                                wait\u003dFalse,"},{"line_number":123,"context_line":"                                **kwargs)"},{"line_number":124,"context_line":""},{"line_number":125,"context_line":"        if domain is None:"},{"line_number":126,"context_line":"            self.exit_json(changed\u003dis_changed)"},{"line_number":127,"context_line":"        else:"},{"line_number":128,"context_line":"            self.exit_json(changed\u003dis_changed,"},{"line_number":129,"context_line":"                           domain\u003ddomain.to_dict(computed\u003dFalse))"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"def main():"}],"source_content_type":"text/x-python","patch_set":3,"id":"628deff9_167ad17d","line":129,"range":{"start_line":125,"start_character":8,"end_line":129,"end_character":64},"in_reply_to":"bb8abb6f_dc3821b4","updated":"2023-01-25 12:53:32.000000000","message":"It\u0027s not consistent because it wasn\u0027t used, Statemachine class is also a new thing. Why isn\u0027t it readable and symmetric?","commit_id":"a00354c1e02475dd998b5cfe1c4adac463dbbc4b"}]}