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.
Tomas
--
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
