)]}'
{"neutron/agent/linux/dhcp.py":[{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"3d742c58af4a7d980793fab9ecf3dc5a0b555069","unresolved":false,"context_lines":[{"line_number":373,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":374,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":375,"context_line":"                    continue"},{"line_number":376,"context_line":"                else:"},{"line_number":377,"context_line":"                    # just skip mode keyword"},{"line_number":378,"context_line":"                    mode \u003d None"},{"line_number":379,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_fde0fbfb","line":376,"updated":"2014-07-03 13:37:05.000000000","message":"why adding else block? Consider setting mode \u003d None at the beginning of the loop (#362)","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":7125,"name":"Alexander Ignatov","email":"aignatov@mirantis.com","username":"aignatov"},"change_message_id":"6e3154e27728d0b74844856f2af86d2b321674dc","unresolved":false,"context_lines":[{"line_number":373,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":374,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":375,"context_line":"                    continue"},{"line_number":376,"context_line":"                else:"},{"line_number":377,"context_line":"                    # just skip mode keyword"},{"line_number":378,"context_line":"                    mode \u003d None"},{"line_number":379,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_b21a2d7b","line":376,"in_reply_to":"baada198_fde0fbfb","updated":"2014-07-03 14:25:08.000000000","message":"Done","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"3d742c58af4a7d980793fab9ecf3dc5a0b555069","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":375,"context_line":"                    continue"},{"line_number":376,"context_line":"                else:"},{"line_number":377,"context_line":"                    # just skip mode keyword"},{"line_number":378,"context_line":"                    mode \u003d None"},{"line_number":379,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":380,"context_line":"                set_tag \u003d \u0027set:\u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_bd33e3a9","line":377,"updated":"2014-07-03 13:37:05.000000000","message":"not clear in this context, suggest to move this comment somewhere near #391, also would be good to add that the mode is actually optional","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":7125,"name":"Alexander Ignatov","email":"aignatov@mirantis.com","username":"aignatov"},"change_message_id":"6e3154e27728d0b74844856f2af86d2b321674dc","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":375,"context_line":"                    continue"},{"line_number":376,"context_line":"                else:"},{"line_number":377,"context_line":"                    # just skip mode keyword"},{"line_number":378,"context_line":"                    mode \u003d None"},{"line_number":379,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":380,"context_line":"                set_tag \u003d \u0027set:\u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_f210255b","line":377,"in_reply_to":"baada198_bd33e3a9","updated":"2014-07-03 14:25:08.000000000","message":"Done","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"3d742c58af4a7d980793fab9ecf3dc5a0b555069","unresolved":false,"context_lines":[{"line_number":388,"context_line":"            else:"},{"line_number":389,"context_line":"                lease \u003d \u0027%ss\u0027 % self.conf.dhcp_lease_duration"},{"line_number":390,"context_line":""},{"line_number":391,"context_line":"            if mode:"},{"line_number":392,"context_line":"                cmd.append(\u0027--dhcp-range\u003d%s%s,%s,%s,%s\u0027 %"},{"line_number":393,"context_line":"                           (set_tag, self._TAG_PREFIX % i,"},{"line_number":394,"context_line":"                            cidr.network, mode, lease))"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_af39eaa5","line":391,"updated":"2014-07-03 13:37:05.000000000","message":"Consider:\n dhcp_range \u003d \u0027--dhcp-range\u003d%s%s,%s,%s,%s\u0027 %\n     (set_tag, self._TAG_PREFIX % i,\n      cidr.network, mode + \u0027,\u0027 if mode else \u0027\u0027, lease)\n cmd.append(dhcp_range)","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":7125,"name":"Alexander Ignatov","email":"aignatov@mirantis.com","username":"aignatov"},"change_message_id":"6e3154e27728d0b74844856f2af86d2b321674dc","unresolved":false,"context_lines":[{"line_number":388,"context_line":"            else:"},{"line_number":389,"context_line":"                lease \u003d \u0027%ss\u0027 % self.conf.dhcp_lease_duration"},{"line_number":390,"context_line":""},{"line_number":391,"context_line":"            if mode:"},{"line_number":392,"context_line":"                cmd.append(\u0027--dhcp-range\u003d%s%s,%s,%s,%s\u0027 %"},{"line_number":393,"context_line":"                           (set_tag, self._TAG_PREFIX % i,"},{"line_number":394,"context_line":"                            cidr.network, mode, lease))"}],"source_content_type":"text/x-python","patch_set":1,"id":"baada198_85e2452d","line":391,"in_reply_to":"baada198_af39eaa5","updated":"2014-07-03 14:25:08.000000000","message":"I don\u0027t think so, with your consideration such code block gets more complicated for reading. It is not obvious why we need to add extra \u0027,\u0027 here until we read \u0027man dnsmasq\u0027.","commit_id":"167cd5ff38e351e0078ba22996f7664068ffa680"},{"author":{"_account_id":1923,"name":"Mohammad Banikazemi","email":"mbanikazemi@gmail.com","username":"mb-s"},"change_message_id":"8d186e95153cffb68c828f584889f12300136ab5","unresolved":false,"context_lines":[{"line_number":359,"context_line":""},{"line_number":360,"context_line":"        possible_leases \u003d 0"},{"line_number":361,"context_line":"        for i, subnet in enumerate(self.network.subnets):"},{"line_number":362,"context_line":"            mode \u003d None"},{"line_number":363,"context_line":"            # if a subnet is specified to have dhcp disabled"},{"line_number":364,"context_line":"            if not subnet.enable_dhcp:"},{"line_number":365,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_baf75f58","line":362,"updated":"2014-07-03 16:23:13.000000000","message":"Code will be more readable if you move this to the if block where \u0027mode\u0027 is being set. please see the suggestion below.","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"},{"author":{"_account_id":5948,"name":"Oleg Bondarev","email":"obondarev@mirantis.com","username":"obondarev"},"change_message_id":"4a07e32abe89704312d42e28a4266b9d668e3d16","unresolved":false,"context_lines":[{"line_number":359,"context_line":""},{"line_number":360,"context_line":"        possible_leases \u003d 0"},{"line_number":361,"context_line":"        for i, subnet in enumerate(self.network.subnets):"},{"line_number":362,"context_line":"            mode \u003d None"},{"line_number":363,"context_line":"            # if a subnet is specified to have dhcp disabled"},{"line_number":364,"context_line":"            if not subnet.enable_dhcp:"},{"line_number":365,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_2b0bee8e","line":362,"in_reply_to":"baada198_baf75f58","updated":"2014-07-04 07:35:05.000000000","message":"My opinion is that it is safer to initialize a variable outside of the conditions. Otherwise it is easier to introduce UnboundLocalError again If later have to define some new conditions or remove existing - in fact that led to the bug which this patch is fixing.","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"},{"author":{"_account_id":1923,"name":"Mohammad Banikazemi","email":"mbanikazemi@gmail.com","username":"mb-s"},"change_message_id":"8d186e95153cffb68c828f584889f12300136ab5","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":375,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":376,"context_line":"                    continue"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":379,"context_line":"                set_tag \u003d \u0027set:\u0027"},{"line_number":380,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_5a8ba3c2","line":377,"updated":"2014-07-03 16:23:13.000000000","message":"imo, it will be easier to follow the logic if the assignment of None to mode happens here (or at least at the beginning of the else block).\n\nelse:\n   mode \u003d None","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"},{"author":{"_account_id":6072,"name":"Eugene Nikanorov","email":"enikanorov@mirantis.com","username":"enikanorov"},"change_message_id":"d26a718129c8c7ac5c69b7f5ea6b75d7a2386cf3","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":375,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":376,"context_line":"                    continue"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":379,"context_line":"                set_tag \u003d \u0027set:\u0027"},{"line_number":380,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_c1e50339","line":377,"in_reply_to":"baada198_5a8ba3c2","updated":"2014-07-03 19:53:41.000000000","message":"That\u0027s funny how different reviewers find different ways of doing the same to be more clean.\n\nI think it\u0027s no big deal. I didn\u0027t deserve -1 previously, it doesn\u0027t deserve changing it in this patchset.","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"be160cd1c714c989d350b2d425855468d1145c7b","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":375,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":376,"context_line":"                    continue"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":379,"context_line":"                set_tag \u003d \u0027set:\u0027"},{"line_number":380,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_b38573d0","line":377,"in_reply_to":"baada198_847959ca","updated":"2014-07-07 18:42:21.000000000","message":"I\u0027m not a bit surprised that different reviewers sometimes see things differently.  It happens.  I see Oleg\u0027s point but I still happen to agree with Mohammad in this case.  That would\u0027ve been my preference.\n\nThis does not merit slowing down the merge process to hash out.  I don\u0027t see a good reason not to approve this change.","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"},{"author":{"_account_id":1923,"name":"Mohammad Banikazemi","email":"mbanikazemi@gmail.com","username":"mb-s"},"change_message_id":"19688b47c3f0fc36d5aedd061c0a131cc7f51fdc","unresolved":false,"context_lines":[{"line_number":374,"context_line":"                elif getattr(subnet, \u0027ipv6_ra_mode\u0027, None) is None:"},{"line_number":375,"context_line":"                    # RA mode is not set - do not launch dnsmasq"},{"line_number":376,"context_line":"                    continue"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"            if self.version \u003e\u003d self.MINIMUM_VERSION:"},{"line_number":379,"context_line":"                set_tag \u003d \u0027set:\u0027"},{"line_number":380,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":2,"id":"baada198_847959ca","line":377,"in_reply_to":"baada198_c1e50339","updated":"2014-07-03 20:41:41.000000000","message":"I now see the comments on the first patch set. I think (surprise, surprise!) I have a point here though. It took me a bit to figure out the non existing \"else\" logic, hence the suggestion. The fate of mode is decided solely in the if/else block. Anyhow, either way is finel.","commit_id":"8479c212b79e61aa9b694ec1368a1073898a09d5"}]}