)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"e4088de3_67157a7c","updated":"2022-05-10 15:02:29.000000000","message":"I have several questions (sometimes rhetorical), but otherwise don\u0027t see any problems.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":33807,"name":"Jacob Wang","email":"jacob_wang1@dell.com","username":"jacob0522"},"change_message_id":"86a219cca9ec7096734c5b28b996092b9fc83037","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"7f96ac06_dbfa48ce","updated":"2022-01-14 08:57:50.000000000","message":"run-DellEMC VNX CI","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"4f74c66fb6efe7f041d798ffe1dd48b7b33d2fac","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"c8423b19_2cbfab9f","updated":"2022-06-09 16:01:12.000000000","message":"LGTM.","commit_id":"e6a264e4ad8ba8520849d800af25d63b30303ccc"},{"author":{"_account_id":20813,"name":"Sofia Enriquez","email":"lsofia.enriquez@gmail.com","username":"enriquetaso"},"change_message_id":"232d165f2e46fec63ea535bb043a315a7d548526","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"48980f08_52083d1a","updated":"2022-07-06 15:39:32.000000000","message":"Looks good but the patch still have open comments. ","commit_id":"e6a264e4ad8ba8520849d800af25d63b30303ccc"},{"author":{"_account_id":20813,"name":"Sofia Enriquez","email":"lsofia.enriquez@gmail.com","username":"enriquetaso"},"change_message_id":"5fcca352e7c6d0eb934ed6291b24b2157e8d335a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"6de69ffb_3b17d7cf","updated":"2022-07-06 15:46:02.000000000","message":"Looks like not! LGTM","commit_id":"e6a264e4ad8ba8520849d800af25d63b30303ccc"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"c8bcdbeb28d0d9fa78fb3efe5df8d2516b8d6590","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"661fa5c2_dd618392","updated":"2022-05-26 14:14:28.000000000","message":"recheck\n\nneeds unit test fix: https://review.opendev.org/c/openstack/cinder/+/843151\n","commit_id":"e6a264e4ad8ba8520849d800af25d63b30303ccc"}],"cinder/service.py":[{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":138,"context_line":"                 service_name: Optional[str] \u003d None,"},{"line_number":139,"context_line":"                 coordination: bool \u003d False,"},{"line_number":140,"context_line":"                 cluster: Optional[str] \u003d None,"},{"line_number":141,"context_line":"                 *args, **kwargs):"},{"line_number":142,"context_line":"        super(Service, self).__init__()"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if not rpc.initialized():"}],"source_content_type":"text/x-python","patch_set":9,"id":"3af63fc8_e51fdcba","line":141,"updated":"2022-05-10 15:02:29.000000000","message":"You could add \"-\u003e None\" for the return type. I don\u0027t see this done in other cinder class __init__(), so I\u0027m not sure if that\u0027s an oversight or if you have a reason for not annotating the return.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"e9effac9863fb18baeac5b03fb18d5094594e714","unresolved":true,"context_lines":[{"line_number":138,"context_line":"                 service_name: Optional[str] \u003d None,"},{"line_number":139,"context_line":"                 coordination: bool \u003d False,"},{"line_number":140,"context_line":"                 cluster: Optional[str] \u003d None,"},{"line_number":141,"context_line":"                 *args, **kwargs):"},{"line_number":142,"context_line":"        super(Service, self).__init__()"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if not rpc.initialized():"}],"source_content_type":"text/x-python","patch_set":9,"id":"5df0040d_bc8d9e51","line":141,"in_reply_to":"3af63fc8_e51fdcba","updated":"2022-05-10 19:34:28.000000000","message":"mypy assumes that the return type for __init__ is None, so I\u0027ve generally skipped adding it.  You can see that this works by adding a return here and running the mypy checks.  It will fail with something like\n\ncinder/service.py:220:9: error: No return value expected  [return-value]","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4e6537dba75c6f8515c0e7fe6c7b94b01c899e8b","unresolved":false,"context_lines":[{"line_number":138,"context_line":"                 service_name: Optional[str] \u003d None,"},{"line_number":139,"context_line":"                 coordination: bool \u003d False,"},{"line_number":140,"context_line":"                 cluster: Optional[str] \u003d None,"},{"line_number":141,"context_line":"                 *args, **kwargs):"},{"line_number":142,"context_line":"        super(Service, self).__init__()"},{"line_number":143,"context_line":""},{"line_number":144,"context_line":"        if not rpc.initialized():"}],"source_content_type":"text/x-python","patch_set":9,"id":"dd735704_2a9b8c5a","line":141,"in_reply_to":"5df0040d_bc8d9e51","updated":"2022-05-11 20:27:53.000000000","message":"Ack, makes sense. And since I sometimes question S/N of some annotations (adds noise for not a lot of value), then I\u0027m in favor of *not* adding \"-\u003e None\" now that I know this is mypy implicit.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":159,"context_line":"                                     cluster\u003dself.cluster,"},{"line_number":160,"context_line":"                                     service_name\u003dservice_name,"},{"line_number":161,"context_line":"                                     *args, **kwargs)"},{"line_number":162,"context_line":"        self.availability_zone: str \u003d self.manager.availability_zone"},{"line_number":163,"context_line":"        self.model_disconnected: bool"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"        # NOTE(geguileo): We need to create the Service DB entry before we"}],"source_content_type":"text/x-python","patch_set":9,"id":"486583b3_5824949c","line":162,"updated":"2022-05-10 15:02:29.000000000","message":"I\u0027m still learning how mypy works, and am not sure what this change will do. I can imagine code in the manager that ensures the availability_zone is a str, so is this just asking mypy to cross-check to ensure the manager did it correctly?","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":160,"context_line":"                                     service_name\u003dservice_name,"},{"line_number":161,"context_line":"                                     *args, **kwargs)"},{"line_number":162,"context_line":"        self.availability_zone: str \u003d self.manager.availability_zone"},{"line_number":163,"context_line":"        self.model_disconnected: bool"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"        # NOTE(geguileo): We need to create the Service DB entry before we"},{"line_number":166,"context_line":"        # create the manager, otherwise capped versions for serializer and rpc"}],"source_content_type":"text/x-python","patch_set":9,"id":"d8a0b8bb_4c1cc969","line":163,"updated":"2022-05-10 15:02:29.000000000","message":"This is another instance of me still learning mypy. Does it retain context across the entire file, and will use this annotation when it encounters L224?","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4e6537dba75c6f8515c0e7fe6c7b94b01c899e8b","unresolved":true,"context_lines":[{"line_number":160,"context_line":"                                     service_name\u003dservice_name,"},{"line_number":161,"context_line":"                                     *args, **kwargs)"},{"line_number":162,"context_line":"        self.availability_zone: str \u003d self.manager.availability_zone"},{"line_number":163,"context_line":"        self.model_disconnected: bool"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"        # NOTE(geguileo): We need to create the Service DB entry before we"},{"line_number":166,"context_line":"        # create the manager, otherwise capped versions for serializer and rpc"}],"source_content_type":"text/x-python","patch_set":9,"id":"757c576f_cacf2805","line":163,"in_reply_to":"6ebcffa5_6fd87f21","updated":"2022-05-11 20:27:53.000000000","message":"Yup, that\u0027s what I was thinking. We\u0027re adding mypy annotation to counteract code that could be better written.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"e9effac9863fb18baeac5b03fb18d5094594e714","unresolved":true,"context_lines":[{"line_number":160,"context_line":"                                     service_name\u003dservice_name,"},{"line_number":161,"context_line":"                                     *args, **kwargs)"},{"line_number":162,"context_line":"        self.availability_zone: str \u003d self.manager.availability_zone"},{"line_number":163,"context_line":"        self.model_disconnected: bool"},{"line_number":164,"context_line":""},{"line_number":165,"context_line":"        # NOTE(geguileo): We need to create the Service DB entry before we"},{"line_number":166,"context_line":"        # create the manager, otherwise capped versions for serializer and rpc"}],"source_content_type":"text/x-python","patch_set":9,"id":"6ebcffa5_6fd87f21","line":163,"in_reply_to":"d8a0b8bb_4c1cc969","updated":"2022-05-10 19:34:28.000000000","message":"Correct, it sets the type information.\n\nBut, this is probably not the best way to do this.  Well-written python code would define self.model_disconnected as a member variable in __init__ and give it a value...","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":282,"context_line":"                              initial_delay\u003dself.report_interval)"},{"line_number":283,"context_line":""},{"line_number":284,"context_line":"        if self.periodic_interval:"},{"line_number":285,"context_line":"            initial_delay: Optional[int]"},{"line_number":286,"context_line":"            if self.periodic_fuzzy_delay:"},{"line_number":287,"context_line":"                initial_delay \u003d random.randint(0, self.periodic_fuzzy_delay)"},{"line_number":288,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":9,"id":"4d4dcdeb_35b218f8","line":285,"updated":"2022-05-10 15:02:29.000000000","message":"Taken on its own, this seems reasonable. I\u0027m a little concerned the cinder code could get unwieldy if we end up annotating every local variable, especially when the variable\u0027s scope is noticeably small.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"e9effac9863fb18baeac5b03fb18d5094594e714","unresolved":true,"context_lines":[{"line_number":282,"context_line":"                              initial_delay\u003dself.report_interval)"},{"line_number":283,"context_line":""},{"line_number":284,"context_line":"        if self.periodic_interval:"},{"line_number":285,"context_line":"            initial_delay: Optional[int]"},{"line_number":286,"context_line":"            if self.periodic_fuzzy_delay:"},{"line_number":287,"context_line":"                initial_delay \u003d random.randint(0, self.periodic_fuzzy_delay)"},{"line_number":288,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":9,"id":"e5cad917_5c89c283","line":285,"in_reply_to":"4d4dcdeb_35b218f8","updated":"2022-05-10 19:34:28.000000000","message":"Typically, this would have been added here because line 287 implies a type of int, and then line 289 fails because it implies a conflicting type of Optional[int].\n\nDoing this allows both cases.  But, running this now w/o line 285 doesn\u0027t seem to generate that error... unclear why.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":4523,"name":"Eric Harney","email":"eharney@redhat.com","username":"eharney"},"change_message_id":"d741e1e407abfd2c99a3859b57bd523542170fa5","unresolved":true,"context_lines":[{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    def _create_service_ref(self,"},{"line_number":349,"context_line":"                            context: context.RequestContext,"},{"line_number":350,"context_line":"                            rpc_version: str \u003d None) -\u003e None:"},{"line_number":351,"context_line":"        kwargs \u003d {"},{"line_number":352,"context_line":"            \u0027host\u0027: self.host,"},{"line_number":353,"context_line":"            \u0027binary\u0027: self.binary,"}],"source_content_type":"text/x-python","patch_set":9,"id":"d73f00d1_2c99cf50","line":350,"range":{"start_line":350,"start_character":41,"end_line":350,"end_character":45},"updated":"2022-05-17 21:24:41.000000000","message":"Needs Optional[] now that we have enabled no_implicit_optional.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":518,"context_line":"class WSGIService(service.ServiceBase):"},{"line_number":519,"context_line":"    \"\"\"Provides ability to launch API from a \u0027paste\u0027 configuration.\"\"\""},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def __init__(self, name, loader\u003dNone):"},{"line_number":522,"context_line":"        \"\"\"Initialize, but do not start the WSGI server."},{"line_number":523,"context_line":""},{"line_number":524,"context_line":"        :param name: The name of the WSGI server given to the loader."}],"source_content_type":"text/x-python","patch_set":9,"id":"9c1d45ec_b7d3cad4","line":521,"updated":"2022-05-10 15:02:29.000000000","message":"Per my previous observation, this is another place where \"-\u003e None\" could be added (though I don\u0027t see a lot of value).","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"4e6537dba75c6f8515c0e7fe6c7b94b01c899e8b","unresolved":false,"context_lines":[{"line_number":518,"context_line":"class WSGIService(service.ServiceBase):"},{"line_number":519,"context_line":"    \"\"\"Provides ability to launch API from a \u0027paste\u0027 configuration.\"\"\""},{"line_number":520,"context_line":""},{"line_number":521,"context_line":"    def __init__(self, name, loader\u003dNone):"},{"line_number":522,"context_line":"        \"\"\"Initialize, but do not start the WSGI server."},{"line_number":523,"context_line":""},{"line_number":524,"context_line":"        :param name: The name of the WSGI server given to the loader."}],"source_content_type":"text/x-python","patch_set":9,"id":"6e25c477_973d8c91","line":521,"in_reply_to":"9c1d45ec_b7d3cad4","updated":"2022-05-11 20:27:53.000000000","message":"Nevermind, just adds noise as it\u0027s implicit.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"},{"author":{"_account_id":21129,"name":"Alan Bishop","email":"abishopsweng@gmail.com","username":"ASBishop","status":"ex Red Hat"},"change_message_id":"aafdb2adcc7adeac140f19b3d51fa53af05596e8","unresolved":true,"context_lines":[{"line_number":657,"context_line":"        return process_launcher()"},{"line_number":658,"context_line":""},{"line_number":659,"context_line":""},{"line_number":660,"context_line":"class WindowsProcessLauncher(object):"},{"line_number":661,"context_line":"    def __init__(self):"},{"line_number":662,"context_line":"        self._processutils \u003d os_win_utilsfactory.get_processutils()"},{"line_number":663,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"22d40637_9684a6c9","line":660,"updated":"2022-05-10 15:02:29.000000000","message":"I know it\u0027s a slippery slope, but none of these are annotated. Again, though, I don\u0027t see a lot of value in plastering the code with annotations that don\u0027t add real value, so I\u0027m OK leaving this class alone for now.","commit_id":"2143a2a37df951ca9f387c3b3553e10e8b9ed2a7"}]}