)]}'
{"barbican/plugin/dogtag.py":[{"author":{"_account_id":7262,"name":"John Vrbanac","email":"john.vrbanac@linux.com","username":"john.vrbanac"},"change_message_id":"d5705615a5fb91a72c34c473c173466b6fa141ba","unresolved":false,"context_lines":[{"line_number":487,"context_line":"            order_id, order_meta, plugin_meta)"},{"line_number":488,"context_line":""},{"line_number":489,"context_line":"        if result_dto.status \u003d\u003d cm.CertificateStatus.REQUEST_CANCELED:"},{"line_number":490,"context_line":"            return self.issue_certificate_request("},{"line_number":491,"context_line":"                order_id, order_meta, plugin_meta)"},{"line_number":492,"context_line":"        elif result_dto.status \u003d\u003d cm.CertificateStatus.INVALID_OPERATION:"},{"line_number":493,"context_line":"            return cm.ResultDTO("}],"source_content_type":"text/x-python","patch_set":2,"id":"1abeadc6_477ba3f4","line":490,"updated":"2014-08-21 22:15:01.000000000","message":"Wouldn\u0027t it be cleaner to set result_dto with the returned value and then return a single result_dto at the end of this method? This pattern of a large number of returns is used quite heavily in this diff, personally, it feels like it would be little easier to read if we were returning just once; unless we\u0027re forcing an early return for error handling or performance concerns. Thoughts?","commit_id":"b6c06e502d61b35dcf3abb5f048199189d85d542"},{"author":{"_account_id":9914,"name":"Ade Lee","email":"alee@redhat.com","username":"alee"},"change_message_id":"aee87e6baaf9309c261a577e5769e39a9ef2cdbe","unresolved":false,"context_lines":[{"line_number":487,"context_line":"            order_id, order_meta, plugin_meta)"},{"line_number":488,"context_line":""},{"line_number":489,"context_line":"        if result_dto.status \u003d\u003d cm.CertificateStatus.REQUEST_CANCELED:"},{"line_number":490,"context_line":"            return self.issue_certificate_request("},{"line_number":491,"context_line":"                order_id, order_meta, plugin_meta)"},{"line_number":492,"context_line":"        elif result_dto.status \u003d\u003d cm.CertificateStatus.INVALID_OPERATION:"},{"line_number":493,"context_line":"            return cm.ResultDTO("}],"source_content_type":"text/x-python","patch_set":2,"id":"1abeadc6_c5a7a6a5","line":490,"in_reply_to":"1abeadc6_477ba3f4","updated":"2014-08-22 02:38:24.000000000","message":"I actually think that this would make things less clear.\nThe point is to return the correct value once the relevant result is returned from the CA, rather than having the reader look through a slew of cases which do not apply.\n\nIn this case, there would be confusion as to whether the result_dto is from the cancel or issue operation.\n\nMaybe it would be clearer to distinguish between the result_dto returned by the cancel() operation and the result_dto returned by the issue_certificate_request() operation, as in the code below .. \n\n\ncancel_result_dto \u003d self.cancel_certificate_request(\n    order_id, order_meta, plugin_meta)\n\nif cancel_result_dto.status \u003d\u003d cm.CertificateStatus.REQUEST_CANCELED:\n    issue_result_dto \u003d self.issue_certificate_request(\n        order_id, order_meta, plugin_meta)\n    return issue_result_dto\nelif cancel_result_dto.status \u003d\u003d cm.CertificateStatus.INVALID_OPERATION:\n    return cm.ResultDTO(\n        cm.CertificateStatus.INVALID_OPERATION,\n        status_message\u003d\"Modify request: unable to cancel: {0}\"\n        .format(result_dto.status_message))\nelse:\n    # other status (ca_unavailable, client_data_issue)\n    # return result from cancel operation\n    return cancel_result_dto\n\nIs that clearer?","commit_id":"b6c06e502d61b35dcf3abb5f048199189d85d542"},{"author":{"_account_id":7262,"name":"John Vrbanac","email":"john.vrbanac@linux.com","username":"john.vrbanac"},"change_message_id":"fa275aea2cbb86985540e2391620ea3d9199d1e7","unresolved":false,"context_lines":[{"line_number":487,"context_line":"            order_id, order_meta, plugin_meta)"},{"line_number":488,"context_line":""},{"line_number":489,"context_line":"        if result_dto.status \u003d\u003d cm.CertificateStatus.REQUEST_CANCELED:"},{"line_number":490,"context_line":"            return self.issue_certificate_request("},{"line_number":491,"context_line":"                order_id, order_meta, plugin_meta)"},{"line_number":492,"context_line":"        elif result_dto.status \u003d\u003d cm.CertificateStatus.INVALID_OPERATION:"},{"line_number":493,"context_line":"            return cm.ResultDTO("}],"source_content_type":"text/x-python","patch_set":2,"id":"1abeadc6_1f5d5479","line":490,"in_reply_to":"1abeadc6_c5a7a6a5","updated":"2014-08-22 15:01:07.000000000","message":"A wee bit better. Technically, I think to get proper context reading this method that the cases do apply as they are a part of the logic flow. Anyhow, this is more of a personal opinion, so I\u0027ll remove my -1, but I would like to revisit this a some point to figure out a clearer way of defining out our logic flows.","commit_id":"b6c06e502d61b35dcf3abb5f048199189d85d542"}]}