On 11/23/2015 01:40 PM, Tomas Babej wrote: > > > On 11/23/2015 01:31 PM, Jan Cholasta wrote: >> On 23.11.2015 13:28, Tomas Babej wrote: >>> >>> >>> On 11/23/2015 01:11 PM, Jan Cholasta wrote: >>>> On 23.11.2015 12:53, Tomas Babej wrote: >>>>> Hi, >>>>> >>>>> If the code within the private_ccache contextmanager does not >>>>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which >>>>> will cause unnecessary termination of the code flow. >>>>> >>>>> Make sure the KRB5CCNAME is popped out of os.environ only if present. >>>>> >>>>> Tomas >>>> >>>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it. >>> >>> Yeah, well, both ways are equivalent, I found the if statement more >>> explicit. We can go with the suggested version, if it's more pleasing >>> though - patch is attached. >>> >>>> >>>> Also I don't think the comment is necessary, it's quite obvious what the >>>> code does. >>>> >>> >>> I don't think an explanatory comment can hurt, ever. Worst thing that >>> happens is that somebody is assured that he understands the code >>> correctly. >> >> Actually the worst thing is that someone changes the code without >> changing the comment. If the comment does not add any real value, it's >> only a maintenance burden. >> > > Yep, that's a real issue in our code base (i.e. I recently removed such > a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making > sure the comments describe the implementation properly is on the > author/reviewer though. > > What's "added value" highly depends on your skill set, and familiarity > with the code base. Particularly the familiarity with the code base can > diminish over time even for the author, and those are the times where > comments can come to the rescue. > > For a newcomer to the project, even a trivial comment (from the point of > view of the experienced developer) can be valuable.
While I agree with Tomas in general, in this particular case I also do not see the value of the comment. All is said in the 4 lines of code, no deep FreeIPA knowledge required: if original_value is not None: os.environ['KRB5CCNAME'] = original_value else: - os.environ.pop('KRB5CCNAME') + # No value was set originally, make sure no value is set now + os.environ.pop('KRB5CCNAME', None) I hope the discussion around the comment does not expand too much, there is enough work in 4.3 to do... -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code