)]}'
{"glance/tests/unit/common/test_wsgi.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"2cb1f0e72800dd6cb85090572581c64d23ea0f8e","unresolved":false,"context_lines":[{"line_number":558,"context_line":"        fixture \u003d {\u0027name\u0027: \u0027fake public image\u0027,"},{"line_number":559,"context_line":"                   \u0027is_public\u0027: True,"},{"line_number":560,"context_line":"                   \u0027deleted\u0027: False,"},{"line_number":561,"context_line":"                   \u0027name\u0027: None,"},{"line_number":562,"context_line":"                   \u0027size\u0027: 19,"},{"line_number":563,"context_line":"                   \u0027location\u0027: \"file:///tmp/glance-tests/2\","},{"line_number":564,"context_line":"                   \u0027properties\u0027: {\u0027distro\u0027: \u0027Ubuntu 10.04 LTS\u0027}}"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_937aec86","side":"PARENT","line":561,"updated":"2015-11-19 15:44:39.000000000","message":"This is here for a reason. Please see the test logic below.","commit_id":"a1c844c89350d69b2cc795d2d807c4476f5559be"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"816e715ed8342b12b652d7ce8bce190a3f7a6ddd","unresolved":false,"context_lines":[{"line_number":558,"context_line":"        fixture \u003d {\u0027name\u0027: \u0027fake public image\u0027,"},{"line_number":559,"context_line":"                   \u0027is_public\u0027: True,"},{"line_number":560,"context_line":"                   \u0027deleted\u0027: False,"},{"line_number":561,"context_line":"                   \u0027name\u0027: None,"},{"line_number":562,"context_line":"                   \u0027size\u0027: 19,"},{"line_number":563,"context_line":"                   \u0027location\u0027: \"file:///tmp/glance-tests/2\","},{"line_number":564,"context_line":"                   \u0027properties\u0027: {\u0027distro\u0027: \u0027Ubuntu 10.04 LTS\u0027}}"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_b947a376","side":"PARENT","line":561,"in_reply_to":"ba8a016a_937aec86","updated":"2015-11-20 11:33:30.000000000","message":"I probably should have mentioned at this point that the one at the start of the fixture definition is redundant as this over writes it.\n\nSo the duplication is real, I just wanted to make sure that we do not remove that one thing that tests the None valued keys not being included.","commit_id":"a1c844c89350d69b2cc795d2d807c4476f5559be"},{"author":{"_account_id":18494,"name":"Ravi Shekhar Jethani","email":"rsjethani@gmail.com","username":"rsjethani"},"change_message_id":"70a5bce4bf9bf90d72744592a27523fdd1d2ded8","unresolved":false,"context_lines":[{"line_number":558,"context_line":"        fixture \u003d {\u0027name\u0027: \u0027fake public image\u0027,"},{"line_number":559,"context_line":"                   \u0027is_public\u0027: True,"},{"line_number":560,"context_line":"                   \u0027deleted\u0027: False,"},{"line_number":561,"context_line":"                   \u0027name\u0027: None,"},{"line_number":562,"context_line":"                   \u0027size\u0027: 19,"},{"line_number":563,"context_line":"                   \u0027location\u0027: \"file:///tmp/glance-tests/2\","},{"line_number":564,"context_line":"                   \u0027properties\u0027: {\u0027distro\u0027: \u0027Ubuntu 10.04 LTS\u0027}}"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_6a94fb43","side":"PARENT","line":561,"in_reply_to":"ba8a016a_b947a376","updated":"2015-11-23 07:29:20.000000000","message":"There is  already a patch[1] and related bug for this hence I removed the change from my patch.\n\nAnd yes the first occurrence of \u0027name\u0027 should be removed.\n\n[1] https://review.openstack.org/210194/","commit_id":"a1c844c89350d69b2cc795d2d807c4476f5559be"}],"glance/tests/unit/test_auth.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"2cb1f0e72800dd6cb85090572581c64d23ea0f8e","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        service \u003d catalog[-1]"},{"line_number":72,"context_line":"        endpoint_list \u003d []"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if not region_list:"},{"line_number":75,"context_line":"            endpoint_list.append(self.base_endpoint)"},{"line_number":76,"context_line":"        else:"},{"line_number":77,"context_line":"            for region in region_list:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_538564cc","line":74,"updated":"2015-11-19 15:44:39.000000000","message":"This changes the meaning of the condition. The condition is referring to empty list not [False, \"\", None, {}] etc. that would satisfy the \u0027not\u0027. Although any of those would break the else below, it\u0027s better to be specific.","commit_id":"8bf891d39aff848b67f60a9c27a7acb99292cc23"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"816e715ed8342b12b652d7ce8bce190a3f7a6ddd","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        service \u003d catalog[-1]"},{"line_number":72,"context_line":"        endpoint_list \u003d []"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if not region_list:"},{"line_number":75,"context_line":"            endpoint_list.append(self.base_endpoint)"},{"line_number":76,"context_line":"        else:"},{"line_number":77,"context_line":"            for region in region_list:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_39d733ef","line":74,"in_reply_to":"ba8a016a_0d7f98e0","updated":"2015-11-20 11:33:30.000000000","message":"As said I\u0027m not saying that this test logic couldn\u0027t be improved, what I\u0027m saying is that \"if region_list \u003d\u003d []:\" is way more explicit than \"if not region_list:\".\n\nIf we would check the type we would also need to check if it\u0027s empty after that.\n\nMaybe I\u0027m missing something here. Mind to explain the driving reason for this change? As I have no intention to let this go just for a sake of a change, if there is actual benefit doing so, great lets put that to the commit message and have the discussion again.","commit_id":"8bf891d39aff848b67f60a9c27a7acb99292cc23"},{"author":{"_account_id":18494,"name":"Ravi Shekhar Jethani","email":"rsjethani@gmail.com","username":"rsjethani"},"change_message_id":"70a5bce4bf9bf90d72744592a27523fdd1d2ded8","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        service \u003d catalog[-1]"},{"line_number":72,"context_line":"        endpoint_list \u003d []"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if not region_list:"},{"line_number":75,"context_line":"            endpoint_list.append(self.base_endpoint)"},{"line_number":76,"context_line":"        else:"},{"line_number":77,"context_line":"            for region in region_list:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_aa33c361","line":74,"in_reply_to":"ba8a016a_39d733ef","updated":"2015-11-23 07:29:20.000000000","message":"The reason is :PEP8 guidelines [1] (bullet point #19)\n\nThe way \u0027region_list\u0027 is being used and the way it is being called suggests that region_list is being used as a list variable. So to check whether a sequence is empty or not PEP8 suggests the same as what I have proposed.\n\nThe proposed change is not changing the meaning of the condition. Yes the change is removing the extra type check.\n\nIf we are worried about wrong type being passed to the method then in my opinion \u0027if region_list \u003d\u003d []\u0027 is also not a complete solution. The complete solution would be to include \u0027isinstance\u0027 check on every argument passed. That doesn\u0027t sound very Pythonic.\n\nNumerous examples can be given where an argument variable is assumed to be of a specific type and used that way. For example in this method[2] \u0027body\u0027 argument is assumed and used as a dictionary. We are not explicitly checking like body \u003d\u003d {} or similar.\n\nIf you still think this is not a valid change then let me know i will abandon the patch.\n\n[1] https://www.python.org/dev/peps/pep-0008/#programming-recommendations\n\n[2]https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/floating_ips.py#L122","commit_id":"8bf891d39aff848b67f60a9c27a7acb99292cc23"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"e9cebaa1df2ddefb0646d1369b7f340166601c70","unresolved":false,"context_lines":[{"line_number":71,"context_line":"        service \u003d catalog[-1]"},{"line_number":72,"context_line":"        endpoint_list \u003d []"},{"line_number":73,"context_line":""},{"line_number":74,"context_line":"        if not region_list:"},{"line_number":75,"context_line":"            endpoint_list.append(self.base_endpoint)"},{"line_number":76,"context_line":"        else:"},{"line_number":77,"context_line":"            for region in region_list:"}],"source_content_type":"text/x-python","patch_set":1,"id":"ba8a016a_0d7f98e0","line":74,"in_reply_to":"ba8a016a_538564cc","updated":"2015-11-20 06:58:00.000000000","message":"Hi Erno,\n\nWant to clear couple of things\n\n1. If region_list strictly be list can we add strict check using type or isinstance?\n\n2. I have verified the calls of add_service and from every call a list is passed to this method so does this make valid change?\n\nPlease correct me if I am wrong.","commit_id":"8bf891d39aff848b67f60a9c27a7acb99292cc23"}]}