)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"d3089130e1014249faab67b52a8c39fca53a2c8f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"efed3f51_4e7bc65b","updated":"2024-08-25 23:57:44.000000000","message":"Adding WIP to avoid merging this before branch creation.","commit_id":"d3726265201edd2fe07dbdd082e71c1d43250a08"},{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"34537d4cd4ff1c0f165ca3ae1fc037bcb343b40c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"22e63308_cd6faf43","updated":"2024-08-26 12:58:05.000000000","message":"LGTM, sorry to not have reviewed it sooner, I missed that patch.\nThe current implementation seems ok, I just made suggestion, see my inline comment.\n\nFeel free to update it or to merge it when the feature freeze will be done.","commit_id":"d3726265201edd2fe07dbdd082e71c1d43250a08"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"08fd4694799155062109744ee3aeafb2f370427e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"86448fb0_0d8192b0","updated":"2024-09-11 02:32:19.000000000","message":"2024.1 branch was created so this may be now ready to be merged.","commit_id":"9575a24796e019fd66f1bb1a5ef0bcbfc167351a"}],"oslo_limit/limit.py":[{"author":{"_account_id":28522,"name":"Hervé Beraud","email":"herveberaud.pro@gmail.com","username":"hberaud"},"change_message_id":"34537d4cd4ff1c0f165ca3ae1fc037bcb343b40c","unresolved":true,"context_lines":[{"line_number":268,"context_line":"        self._region_id \u003d self._endpoint.region_id"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"    def _get_endpoint(self):"},{"line_number":271,"context_line":"        endpoint_id \u003d CONF.oslo_limit.endpoint_id"},{"line_number":272,"context_line":"        if endpoint_id:"},{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                endpoint \u003d self.connection.get_endpoint(endpoint_id)"},{"line_number":275,"context_line":"            except os_exceptions.ResourceNotFound:"},{"line_number":276,"context_line":"                raise ValueError(\"Can\u0027t find endpoint for %s\" % endpoint_id)"},{"line_number":277,"context_line":"            return endpoint"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"        service_type \u003d CONF.oslo_limit.endpoint_service_type"},{"line_number":280,"context_line":"        service_name \u003d CONF.oslo_limit.endpoint_service_name"}],"source_content_type":"text/x-python","patch_set":2,"id":"9a09a167_dff752cc","line":277,"range":{"start_line":271,"start_character":8,"end_line":277,"end_character":27},"updated":"2024-08-26 12:58:05.000000000","message":"let me know if I\u0027m wrong, if I well understood this patch, if the `endpoint_id` is given by user then the lookup below is ignored else, if the `endpoint_id` is not passed by user the lookup will be made, so it is acceptable to have an empty `endpoint_id`, exact?\n\nI suppose this is why you removed the `if not endpoint_id` from the original implementation, exact?\n\nSo if my analyze is correct, then, I\u0027d suggest to split this method in two sub-parts:\n- one related to the `endpoint_id`\n- and one related to the lookup\n\nBoth parts can be moved to dedicated sub-methods (by example named `get_endpoint_by_id(` and `get_endpoint_by_service_lookup(`) and `_get_endpoint` would be more oriented on the decision logic. Decision to use id or lookup.\n\nI think it will give more readability/clarity to the implementation of `_get_endpoint`.\n\nThis is just a suggestion, so feel free to ignore this comment if you feel the current implementation is ok.","commit_id":"d3726265201edd2fe07dbdd082e71c1d43250a08"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"4ec2f330f4990a63527068e790ed15af103ed1cd","unresolved":true,"context_lines":[{"line_number":268,"context_line":"        self._region_id \u003d self._endpoint.region_id"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"    def _get_endpoint(self):"},{"line_number":271,"context_line":"        endpoint_id \u003d CONF.oslo_limit.endpoint_id"},{"line_number":272,"context_line":"        if endpoint_id:"},{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                endpoint \u003d self.connection.get_endpoint(endpoint_id)"},{"line_number":275,"context_line":"            except os_exceptions.ResourceNotFound:"},{"line_number":276,"context_line":"                raise ValueError(\"Can\u0027t find endpoint for %s\" % endpoint_id)"},{"line_number":277,"context_line":"            return endpoint"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"        service_type \u003d CONF.oslo_limit.endpoint_service_type"},{"line_number":280,"context_line":"        service_name \u003d CONF.oslo_limit.endpoint_service_name"}],"source_content_type":"text/x-python","patch_set":2,"id":"cfbaadd5_f02772f1","line":277,"range":{"start_line":271,"start_character":8,"end_line":277,"end_character":27},"in_reply_to":"9a09a167_dff752cc","updated":"2024-09-05 02:07:22.000000000","message":"\u003e let me know if I\u0027m wrong, if I well understood this patch, if the `endpoint_id` is given by user then the lookup below is ignored else, if the `endpoint_id` is not passed by user the lookup will be made, so it is acceptable to have an empty `endpoint_id`, exact?\n\u003e \n\u003e I suppose this is why you removed the `if not endpoint_id` from the original implementation, exact?\n\nYes, you are correct.\nHowever I now think that it\u0027s probably better to distinguish empty endpoint_id is endpoint_id being unset, because an empty endpoint_id indicates that user explicitly request that value (although it\u0027s apparently invalid, it\u0027s better not to ignore input, IMO). I\u0027ll update the check logic to more explicit one in next version.\n\n\u003e \n\u003e So if my analyze is correct, then, I\u0027d suggest to split this method in two sub-parts:\n\u003e - one related to the `endpoint_id`\n\u003e - and one related to the lookup\n\u003e \n\u003e Both parts can be moved to dedicated sub-methods (by example named `get_endpoint_by_id(` and `get_endpoint_by_service_lookup(`) and `_get_endpoint` would be more oriented on the decision logic. Decision to use id or lookup.\n\u003e \n\u003e I think it will give more readability/clarity to the implementation of `_get_endpoint`.\n\u003e \n\u003e This is just a suggestion, so feel free to ignore this comment if you feel the current implementation is ok.\n\nSounds good to me. I\u0027ll do that split in the next version.","commit_id":"d3726265201edd2fe07dbdd082e71c1d43250a08"},{"author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"change_message_id":"aa6cf9b050caa687a6053d96ebead790fa8f5a29","unresolved":false,"context_lines":[{"line_number":268,"context_line":"        self._region_id \u003d self._endpoint.region_id"},{"line_number":269,"context_line":""},{"line_number":270,"context_line":"    def _get_endpoint(self):"},{"line_number":271,"context_line":"        endpoint_id \u003d CONF.oslo_limit.endpoint_id"},{"line_number":272,"context_line":"        if endpoint_id:"},{"line_number":273,"context_line":"            try:"},{"line_number":274,"context_line":"                endpoint \u003d self.connection.get_endpoint(endpoint_id)"},{"line_number":275,"context_line":"            except os_exceptions.ResourceNotFound:"},{"line_number":276,"context_line":"                raise ValueError(\"Can\u0027t find endpoint for %s\" % endpoint_id)"},{"line_number":277,"context_line":"            return endpoint"},{"line_number":278,"context_line":""},{"line_number":279,"context_line":"        service_type \u003d CONF.oslo_limit.endpoint_service_type"},{"line_number":280,"context_line":"        service_name \u003d CONF.oslo_limit.endpoint_service_name"}],"source_content_type":"text/x-python","patch_set":2,"id":"c2dd0a2c_c4d5a24f","line":277,"range":{"start_line":271,"start_character":8,"end_line":277,"end_character":27},"in_reply_to":"cfbaadd5_f02772f1","updated":"2024-09-05 02:09:39.000000000","message":"Done","commit_id":"d3726265201edd2fe07dbdd082e71c1d43250a08"}]}