)]}'
{"elements/package-installs/bin/package-installs-squash":[{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"3d5b3c5ee57c7e494b7398e875059970964fb369","unresolved":false,"context_lines":[{"line_number":49,"context_line":"        valid_installtype \u003d (installtype is None or"},{"line_number":50,"context_line":"                             installtype \u003d\u003d elem_installtype)"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        # Filter out incorrect ARCH versions"},{"line_number":53,"context_line":"        valid_arch \u003d False"},{"line_number":54,"context_line":"        arch \u003d params.get(\u0027arch\u0027, None)"},{"line_number":55,"context_line":"        not_arch \u003d params.get(\u0027not-arch\u0027, None)"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"3aaa91ec_36adb1e9","line":52,"updated":"2016-06-30 07:01:47.000000000","message":"What about factoring out the next lines into a separate function that returns \u0027valid_arch\u0027?","commit_id":"a9357e302aac6a3b6ced4ed75e6cf0ecfc8553ec"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"3d5b3c5ee57c7e494b7398e875059970964fb369","unresolved":false,"context_lines":[{"line_number":63,"context_line":"                arch \u003d arch.split(\u0027,\u0027)"},{"line_number":64,"context_line":"                map(str.strip, arch)"},{"line_number":65,"context_line":"                if os.environ[\u0027ARCH\u0027] in arch:"},{"line_number":66,"context_line":"                    valid_arch \u003d True"},{"line_number":67,"context_line":"            else:"},{"line_number":68,"context_line":"                # if we don\u0027t have an explicit arch list, we should"},{"line_number":69,"context_line":"                # install unless we are in the not-arch list.  (we"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"3aaa91ec_96e4653c","line":66,"updated":"2016-06-30 07:01:47.000000000","message":"What do you think about checking if the user (accidentally) specified the same arch in arch and not-arch?","commit_id":"a9357e302aac6a3b6ced4ed75e6cf0ecfc8553ec"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"3d5b3c5ee57c7e494b7398e875059970964fb369","unresolved":false,"context_lines":[{"line_number":67,"context_line":"            else:"},{"line_number":68,"context_line":"                # if we don\u0027t have an explicit arch list, we should"},{"line_number":69,"context_line":"                # install unless we are in the not-arch list.  (we"},{"line_number":70,"context_line":"                # must have a not-arch list, else how did we get"},{"line_number":71,"context_line":"                # here?)"},{"line_number":72,"context_line":"                assert(not_arch is not None)"},{"line_number":73,"context_line":"                not_arch \u003d not_arch.split(\u0027,\u0027)"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"3aaa91ec_36f771d4","line":70,"updated":"2016-06-30 07:01:47.000000000","message":"If you ask this yourself here, maybe the structure is not that optimal ;-)\n\nWhat do you think about:\n\nif arch:\n   ...\nelif not_arch:\n   ...\nelse:\n   ...","commit_id":"a9357e302aac6a3b6ced4ed75e6cf0ecfc8553ec"},{"author":{"_account_id":7118,"name":"Ian Wienand","email":"iwienand@redhat.com","username":"iwienand"},"change_message_id":"9ed601dd833136481c049675adba4679e486a0e2","unresolved":false,"context_lines":[{"line_number":67,"context_line":"            else:"},{"line_number":68,"context_line":"                # if we don\u0027t have an explicit arch list, we should"},{"line_number":69,"context_line":"                # install unless we are in the not-arch list.  (we"},{"line_number":70,"context_line":"                # must have a not-arch list, else how did we get"},{"line_number":71,"context_line":"                # here?)"},{"line_number":72,"context_line":"                assert(not_arch is not None)"},{"line_number":73,"context_line":"                not_arch \u003d not_arch.split(\u0027,\u0027)"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"3aaa91ec_a1748867","line":70,"in_reply_to":"3aaa91ec_36f771d4","updated":"2016-07-01 01:51:02.000000000","message":"ok, this was just a throw-away comment to show the assert() was considered.\n\nfeel free to refactor this and reupload if you feel strongly ... personally i feel like this is fairly readable because it deals with the usual case first, and rearranging it is much-of-a-muchness.","commit_id":"a9357e302aac6a3b6ced4ed75e6cf0ecfc8553ec"},{"author":{"_account_id":21741,"name":"Andreas Florath","email":"andreas@florath.net","username":"florath"},"change_message_id":"3d5b3c5ee57c7e494b7398e875059970964fb369","unresolved":false,"context_lines":[{"line_number":70,"context_line":"                # must have a not-arch list, else how did we get"},{"line_number":71,"context_line":"                # here?)"},{"line_number":72,"context_line":"                assert(not_arch is not None)"},{"line_number":73,"context_line":"                not_arch \u003d not_arch.split(\u0027,\u0027)"},{"line_number":74,"context_line":"                map(str.strip, not_arch)"},{"line_number":75,"context_line":"                if os.environ[\u0027ARCH\u0027] not in not_arch:"},{"line_number":76,"context_line":"                    valid_arch \u003d True"}],"source_content_type":"application/octet-stream","patch_set":3,"id":"3aaa91ec_b6bfe1ce","line":73,"updated":"2016-06-30 07:01:47.000000000","message":"Some kind of code duplication (see lines 63-66); I know it\u0027s only four lines, but I find something like\n  return _check_for_arch(arch)\nor\n  return _check_for_arch(not_arch)\nmuch easier to read.","commit_id":"a9357e302aac6a3b6ced4ed75e6cf0ecfc8553ec"},{"author":{"_account_id":6488,"name":"Clint Byrum","email":"clint@fewbar.com","username":"clint-fewbar"},"change_message_id":"0963521d83698000c451f4b265c5d25e6371dcc4","unresolved":false,"context_lines":[{"line_number":52,"context_line":"        return _is_arch_in_list(arch)"},{"line_number":53,"context_line":"    # if we don\u0027t have an explicit arch list, we should"},{"line_number":54,"context_line":"    # install unless we are in the not-arch list."},{"line_number":55,"context_line":"    return not _is_arch_in_list(not_arch)"},{"line_number":56,"context_line":""},{"line_number":57,"context_line":""},{"line_number":58,"context_line":"def collect_data(data, filename, element_name):"}],"source_content_type":"application/octet-stream","patch_set":4,"id":"1aa78d24_b91e245d","line":55,"updated":"2016-07-14 17:47:22.000000000","message":"This is great code. I wonder if we could get it into diskimage_builder and add some unit tests. Sorry.. I get an untested-twitch sometimes. #notaminusone","commit_id":"8a1c8370a13ff5d2eb92b12d3ca7850d0cc6c46d"}]}