)]}'
{"cinder/volume/drivers/dell_emc/powermax/common.py":[{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"9285de6ca822aff6895297f8f50dbc16c9538229","unresolved":true,"context_lines":[{"line_number":3867,"context_line":""},{"line_number":3868,"context_line":"        return False"},{"line_number":3869,"context_line":""},{"line_number":3870,"context_line":"    def _migrate_volume("},{"line_number":3871,"context_line":"            self, array, volume, device_id, srp, target_slo,"},{"line_number":3872,"context_line":"            target_workload, volume_name, new_type, extra_specs):"},{"line_number":3873,"context_line":"        \"\"\"Migrate from one slo/workload combination to another."}],"source_content_type":"text/x-python","patch_set":1,"id":"87f0bf55_0b01d704","line":3870,"updated":"2020-11-25 15:07:55.000000000","message":"I am getting the following from pep8. \n./cinder/volume/drivers/dell_emc/powermax/common.py:3871:5: C901 \u0027PowerMaxCommon._migrate_volume\u0027 is too complex (31)\n\nC901\nFunctions that are deemed too complex are functions that have too much branching logic. Branching logic includes if/elif/else and for/while loops.\n\nIt is a large method so is a candidate for refactoring for readability.","commit_id":"2de71ba280bec93019d461846602cf2b1cdd54b3"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"222791e4e4b1127524997cccaecc9f8785d7953a","unresolved":true,"context_lines":[{"line_number":2054,"context_line":"        self._cleanup_device_snapvx(array, device_id, extra_specs)"},{"line_number":2055,"context_line":"        # Confirm volume has no more snapshots associated and is not a target"},{"line_number":2056,"context_line":"        snapshots \u003d self.rest.get_volume_snapshot_list(array, device_id)"},{"line_number":2057,"context_line":"        __, snapvx_target_details \u003d self.rest.find_snap_vx_sessions("},{"line_number":2058,"context_line":"            array, device_id, tgt_only\u003dTrue)"},{"line_number":2059,"context_line":"        if snapshots:"},{"line_number":2060,"context_line":"            snapshot_names \u003d \u0027, \u0027.join("},{"line_number":2061,"context_line":"                snap.get(\u0027snapshotName\u0027) for snap in snapshots)"}],"source_content_type":"text/x-python","patch_set":4,"id":"a64fa824_dc9029d2","line":2058,"range":{"start_line":2057,"start_character":0,"end_line":2058,"end_character":44},"updated":"2021-05-13 04:00:44.000000000","message":"It would be a slight optimization to move this to after the \u0027if snapshots\u0027 block -- you only have to make this REST API call in the event that there are no snapshots.","commit_id":"1a267bf6ff97d0a36f03119c2e4197baf681c3f5"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"2b759a6e62499262a667c39c00b59a986bd4c5b4","unresolved":true,"context_lines":[{"line_number":2933,"context_line":"        return source_device_id"},{"line_number":2934,"context_line":""},{"line_number":2935,"context_line":"    @retry(retry_exc_tuple, interval\u003d1, retries\u003d3)"},{"line_number":2936,"context_line":"    def _cleanup_device_snapvx("},{"line_number":2937,"context_line":"            self, array, device_id, extra_specs):"},{"line_number":2938,"context_line":"        \"\"\"Perform any snapvx cleanup before creating clones or snapshots"},{"line_number":2939,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"4c407ec6_22757932","line":2936,"range":{"start_line":2936,"start_character":9,"end_line":2936,"end_character":30},"updated":"2021-05-13 21:34:23.000000000","message":"This is a much better name -- way more clear about what\u0027s going on in this function.","commit_id":"1a267bf6ff97d0a36f03119c2e4197baf681c3f5"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"222791e4e4b1127524997cccaecc9f8785d7953a","unresolved":true,"context_lines":[{"line_number":4068,"context_line":"                utils.REPLICATION_DEVICE_BACKEND_ID,"},{"line_number":4069,"context_line":"                utils.BACKEND_ID_LEGACY_REP)"},{"line_number":4070,"context_line":"            backend_ids_differ \u003d curr_backend_id !\u003d tgt_backend_id"},{"line_number":4071,"context_line":""},{"line_number":4072,"context_line":"        return (was_rep_enabled, is_rep_enabled, backend_ids_differ, rep_mode,"},{"line_number":4073,"context_line":"                target_extra_specs)"},{"line_number":4074,"context_line":""},{"line_number":4075,"context_line":"    def _prep_non_rep_to_rep("},{"line_number":4076,"context_line":"            self, array, device_id, volume, was_rep_enabled,"}],"source_content_type":"text/x-python","patch_set":4,"id":"35db797c_3316e397","line":4073,"range":{"start_line":4071,"start_character":0,"end_line":4073,"end_character":35},"updated":"2021-05-13 04:00:44.000000000","message":"this looks like an accident waiting to happen because the caller has to know the exact position of each value.  A lightweight way to make this safer would be something like this:\n\n    from collections import namedtuple\n    def _get_replication_flags(args):\n        rf \u003d namedtuple(\u0027ReplicationFlags\u0027,\n                        \u0027was_rep_enabled, is_rep_enabled, \u0027\n                        \u0027backend_ids_differ, rep_mode, target_extra_specs\u0027)\n        # load it up using your current logic\n        rf.was_rep_enabled \u003d True\n        rf.is_rep_enabled \u003d False\n        rf.backend_ids_differ \u003d True\n        rf.rep_mode \u003d \u0027fancy rep\u0027\n        rf.target_extra_specs \u003d {\u0027k1\u0027:\u0027v1\u0027, \u0027k2\u0027:\u0027v2\u0027}\n        return rf\n\nthen at lines 3907-3910 you could do\n\n    flags \u003d _get_replication_flags(args)\n\nand then use flags.was_rep_enabled, etc., throughout the rest of the function.","commit_id":"1a267bf6ff97d0a36f03119c2e4197baf681c3f5"},{"author":{"_account_id":12670,"name":"Helen Walsh","email":"helen.walsh@emc.com","username":"walshh2"},"change_message_id":"acaf31658038d0252e85712704138087ade084b0","unresolved":true,"context_lines":[{"line_number":4068,"context_line":"                utils.REPLICATION_DEVICE_BACKEND_ID,"},{"line_number":4069,"context_line":"                utils.BACKEND_ID_LEGACY_REP)"},{"line_number":4070,"context_line":"            backend_ids_differ \u003d curr_backend_id !\u003d tgt_backend_id"},{"line_number":4071,"context_line":""},{"line_number":4072,"context_line":"        return (was_rep_enabled, is_rep_enabled, backend_ids_differ, rep_mode,"},{"line_number":4073,"context_line":"                target_extra_specs)"},{"line_number":4074,"context_line":""},{"line_number":4075,"context_line":"    def _prep_non_rep_to_rep("},{"line_number":4076,"context_line":"            self, array, device_id, volume, was_rep_enabled,"}],"source_content_type":"text/x-python","patch_set":4,"id":"7c84059f_bb5c6cae","line":4073,"range":{"start_line":4071,"start_character":0,"end_line":4073,"end_character":35},"in_reply_to":"22dabd39_2ec31452","updated":"2021-05-13 14:06:16.000000000","message":"Thanks for the feedback Brian and Gorka.  This was a first pass refactor as I had issues with the original method being to large and complicated.  Any feedback to improve, most appreciated.\nSome of the other refactored methods below also return tuples, so I am assuming a similar tactic here.","commit_id":"1a267bf6ff97d0a36f03119c2e4197baf681c3f5"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"6de8c286ff9e1984b609a96359117ad5d2700aa0","unresolved":true,"context_lines":[{"line_number":4068,"context_line":"                utils.REPLICATION_DEVICE_BACKEND_ID,"},{"line_number":4069,"context_line":"                utils.BACKEND_ID_LEGACY_REP)"},{"line_number":4070,"context_line":"            backend_ids_differ \u003d curr_backend_id !\u003d tgt_backend_id"},{"line_number":4071,"context_line":""},{"line_number":4072,"context_line":"        return (was_rep_enabled, is_rep_enabled, backend_ids_differ, rep_mode,"},{"line_number":4073,"context_line":"                target_extra_specs)"},{"line_number":4074,"context_line":""},{"line_number":4075,"context_line":"    def _prep_non_rep_to_rep("},{"line_number":4076,"context_line":"            self, array, device_id, volume, was_rep_enabled,"}],"source_content_type":"text/x-python","patch_set":4,"id":"22dabd39_2ec31452","line":4073,"range":{"start_line":4071,"start_character":0,"end_line":4073,"end_character":35},"in_reply_to":"35db797c_3316e397","updated":"2021-05-13 13:03:08.000000000","message":"I think it\u0027s a good idea, but if this is implemented I think the named tuple should be declared globally (as if it were a class) so it can be easily reused.\n\n    from collections import namedtuple\n    ReplicationFlags \u003d namedtuple(\u0027ReplicationFlags\u0027,\n                                  \u0027was_rep_enabled, is_rep_enabled, \u0027\n                                  \u0027backend_ids_differ, rep_mode, target_extra_specs\u0027)\n\n    def _get_replication_flags(args):\n        rf \u003d ReplicationFlags(was_rep_enabled\u003dTrue,\n                              is_rep_enabled\u003dFalse,\n                              backend_ids_differ\u003dTrue,\n                              rep_mode\u003d\u0027fancy rep\u0027,\n                              target_extra_specs\u003d{\u0027k1\u0027:\u0027v1\u0027, \u0027k2\u0027:\u0027v2\u0027})\n        return rf\n\nThe reason for that is that with that implementation is treating the subclass returned by namedtuple as an instance, and it is forcefully replacing the subclass\u0027 properties (was_rep_enabled, is_resp_enabled, etc) with values.\n\nSuggested implementation is no different than doing this:\n\n  rf \u003d namedtuple(\u0027RF\u0027, [])\n  rf.was_rep_enabled \u003d True\n  rf.is_rep_enabled \u003d False\n  rf.anythinggoes \u003d 123\n\nAfter all a tuple should not change its values after it has been created.\n\nPS: Brian\u0027s code works fine btw.","commit_id":"1a267bf6ff97d0a36f03119c2e4197baf681c3f5"}]}