)]}'
{"openstack/common/imageutils.py":[{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"6505e4c1ba42d0e6adc200de996e9dddbbb7afc1","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_1c191688","line":104,"updated":"2014-10-16 02:05:03.000000000","message":"So this is actually logically different then the previous code in that this mutates the lines_after *while* doing a conditional instead of only mutating the lines *only* if the conditional passes. I assume this is ok, but it is worth noting...\n\nIs there any reason not to just replace del lines_after[0] with lines_after.pop(0) instead?","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"41c3c842180de330c6d1ad2e5f5dcbdebe8f9c22","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_f751cbd9","line":104,"in_reply_to":"baa201ad_1c191688","updated":"2014-10-16 02:32:35.000000000","message":"lines_after[0] is going to be removed anyway. So I believe it\u0027s nice to take advantage of pop() method and avoid the unnecessary line (del lines_after[0]). I think using pop(0) keeps the code simpler and more readable.","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"2a80e7ac77145d2b4a5c5bc82472eb06dfafec8d","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_92d32dd4","line":104,"in_reply_to":"baa201ad_f751cbd9","updated":"2014-10-16 04:26:45.000000000","message":"Ok, this is safe since this raises an exception, otherwise it wouldn\u0027t be safe for the same reason described below, \u0027lines_after\u0027 is \u0027lines\u0027 in the _parse loop and modifying that list/array here modifies that same list/array there (since they are the same list/arry)","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"6505e4c1ba42d0e6adc200de996e9dddbbb7afc1","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after.pop(0)"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_3700b300","line":112,"updated":"2014-10-16 02:05:03.000000000","message":"So this actually another bigger change, that is more logically different imho.\n\nThis one mutates the lines_after even if the breaks occur, where previously the mutation only occurred if the breaks did not get triggered. Is this really what you want to have happen? Are other consumers of lines_after expecting those lines to exist (which now won\u0027t exist due to this pop).","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"44de15fe8e08a0bec036bffd212fb2b8dd332c90","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after.pop(0)"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_7f0be2d5","line":112,"in_reply_to":"baa201ad_2ebbf20f","updated":"2014-10-16 15:52:17.000000000","message":"Hmmm, its hard to tell assumptions over code (without comments), this might be a good assumption though or it might not be (I\u0027m not sure), but it seems safer to not assume things if we can (since the creators of qemu-img are really the people who decide if our assumptions are right or wrong).","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"41c3c842180de330c6d1ad2e5f5dcbdebe8f9c22","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after.pop(0)"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_375fd3e6","line":112,"in_reply_to":"baa201ad_3700b300","updated":"2014-10-16 02:32:35.000000000","message":"I think there\u0027s no problem in changing lines_after, because this current method (_extract_details) is called only once, in line 149. After that, the object passed as lines_after is not used anymore inside _parse method.","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":1297,"name":"Joshua Harlow","email":"jxharlow@godaddy.com","username":"harlowja"},"change_message_id":"c751c3d4f346cbcb9497b0200e478f93673bf22a","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after.pop(0)"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_f2739913","line":112,"in_reply_to":"baa201ad_375fd3e6","updated":"2014-10-16 04:23:12.000000000","message":"I\u0027m confused, \n\n\u0027lines_after\u0027 is \u0027lines\u0027 (python is pass by reference) which is used after 149 (since 149 is in a loop), and removing things from \u0027lines_after\u0027 now affects the further usage of \u0027lines\u0027 variable in that loop (which seems unintended).\n\nSo it\u0027s not just called once when its called in a loop (and modifying the lines_after in this method with pop affects how that loop operates).","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"af37af6e5832d91108ff0d70884b3b289b96e3c2","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after.pop(0)"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":6,"id":"baa201ad_2ebbf20f","line":112,"in_reply_to":"baa201ad_f2739913","updated":"2014-10-16 14:18:29.000000000","message":"To be honest, I assumed that snapshot list would be the last relevant information in a QemuImgInfo. If that was the case, there was no problem in using pop(0) inside _extract_details, because _parse would call it later and the conditional \"if top_level\" wouldn\u0027t be satisfied. As it is not necessarily the last information, I\u0027m changing my code. Thanks!","commit_id":"f2e32f0d51524e083be9299240a844738014c6b0"},{"author":{"_account_id":2472,"name":"Doug Hellmann","email":"dhellmann@redhat.com","username":"doug-hellmann"},"change_message_id":"8d535b771436d7e54d288d469ecb6fd89ea228d5","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_21aa1c85","line":104,"updated":"2014-10-16 18:08:51.000000000","message":"This changes the behavior of the function. Now the lines_after list is modified even if an error is reported. Do any callers depend on the current behavior where the list is only modified if there is no error?","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":2472,"name":"Doug Hellmann","email":"dhellmann@redhat.com","username":"doug-hellmann"},"change_message_id":"3e7bfa4f0ef77798092175f05e4208bff0904926","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_2c085327","line":104,"in_reply_to":"baa201ad_019b5818","updated":"2014-10-16 18:30:03.000000000","message":"OK.","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"b2aa0460e08636d905c5a83e69a3746b1bc73cff","unresolved":false,"context_lines":[{"line_number":101,"context_line":"            real_details \u003d real_details.strip().lower()"},{"line_number":102,"context_line":"        elif root_cmd \u003d\u003d \u0027snapshot_list\u0027:"},{"line_number":103,"context_line":"            # Next line should be a header, starting with \u0027ID\u0027"},{"line_number":104,"context_line":"            if not lines_after or not lines_after.pop(0).startswith(\"ID\"):"},{"line_number":105,"context_line":"                msg \u003d _(\"Snapshot list encountered but no header found!\")"},{"line_number":106,"context_line":"                raise ValueError(msg)"},{"line_number":107,"context_line":"            real_details \u003d []"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_019b5818","line":104,"in_reply_to":"baa201ad_21aa1c85","updated":"2014-10-16 18:24:18.000000000","message":"There is only one caller. It\u0027s in a loop inside _parse method and it does not handle the ValueError exception, so I believe the list can be notified with no worries.","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":2472,"name":"Doug Hellmann","email":"dhellmann@redhat.com","username":"doug-hellmann"},"change_message_id":"8d535b771436d7e54d288d469ecb6fd89ea228d5","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after[0]"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_a19d2c68","line":112,"updated":"2014-10-16 18:08:51.000000000","message":"line \u003d lines_after.pop(0)","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":2472,"name":"Doug Hellmann","email":"dhellmann@redhat.com","username":"doug-hellmann"},"change_message_id":"3e7bfa4f0ef77798092175f05e4208bff0904926","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after[0]"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_ac3283ef","line":112,"in_reply_to":"baa201ad_61dd04e0","updated":"2014-10-16 18:30:03.000000000","message":"Ah, good point, that\u0027s the same issue I pointed out above with the error handling.","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"b2aa0460e08636d905c5a83e69a3746b1bc73cff","unresolved":false,"context_lines":[{"line_number":109,"context_line":"            # \"%-10s%-20s%7s%20s%15s\""},{"line_number":110,"context_line":"            # ID TAG VM SIZE DATE VM CLOCK (current header)"},{"line_number":111,"context_line":"            while lines_after:"},{"line_number":112,"context_line":"                line \u003d lines_after[0]"},{"line_number":113,"context_line":"                line_pieces \u003d line.split()"},{"line_number":114,"context_line":"                if len(line_pieces) !\u003d 6:"},{"line_number":115,"context_line":"                    break"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_61dd04e0","line":112,"in_reply_to":"baa201ad_a19d2c68","updated":"2014-10-16 18:24:18.000000000","message":"My patch 6 was as you suggested, but I was advised by Joshua Harlow to change this, because if line 112 is line \u003d lines_after.pop(0), the list will be modified even if \"break\" is called.  I (incorrectly?) assumed that snapshot list would be the last relevant information in a QemuImgInfo. If that was the case, there was no problem in using pop(0) inside _extract_details, because _parse would call it later and the conditional \"if top_level\" wouldn\u0027t be satisfied. As it is not necessarily the last information, I changed the code following his advise.","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":2472,"name":"Doug Hellmann","email":"dhellmann@redhat.com","username":"doug-hellmann"},"change_message_id":"8d535b771436d7e54d288d469ecb6fd89ea228d5","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                date_pieces \u003d line_pieces[5].split(\":\")"},{"line_number":119,"context_line":"                if len(date_pieces) !\u003d 3:"},{"line_number":120,"context_line":"                    break"},{"line_number":121,"context_line":"                lines_after.pop(0)"},{"line_number":122,"context_line":"                real_details.append({"},{"line_number":123,"context_line":"                    \u0027id\u0027: line_pieces[0],"},{"line_number":124,"context_line":"                    \u0027tag\u0027: line_pieces[1],"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_e18ad419","line":121,"updated":"2014-10-16 18:08:51.000000000","message":"This should probably move up to 112 (see previous comment).","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"},{"author":{"_account_id":13463,"name":"Amaury Medeiros","email":"amaurymedeiros@gmail.com","username":"amaurymedeiros"},"change_message_id":"b2aa0460e08636d905c5a83e69a3746b1bc73cff","unresolved":false,"context_lines":[{"line_number":118,"context_line":"                date_pieces \u003d line_pieces[5].split(\":\")"},{"line_number":119,"context_line":"                if len(date_pieces) !\u003d 3:"},{"line_number":120,"context_line":"                    break"},{"line_number":121,"context_line":"                lines_after.pop(0)"},{"line_number":122,"context_line":"                real_details.append({"},{"line_number":123,"context_line":"                    \u0027id\u0027: line_pieces[0],"},{"line_number":124,"context_line":"                    \u0027tag\u0027: line_pieces[1],"}],"source_content_type":"text/x-python","patch_set":7,"id":"baa201ad_219e9c07","line":121,"in_reply_to":"baa201ad_e18ad419","updated":"2014-10-16 18:24:18.000000000","message":"See previous comment.","commit_id":"b2d35eecc1704328f55082ebdaae8004f029187a"}]}