Tomas Babej wrote:
>
>
> On 11/23/2015 01:50 PM, Jan Cholasta wrote:
>> On 23.11.2015 13:40, 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.
>>
>> Let's take a look at the code:
>>
>> if original_value is not None:
>> os.environ['KRB5CCNAME'] = original_value
>> else:
>> os.environ.pop('KRB5CCNAME', None)
>>
>> Can you tell me what in there couldn't be obvious to a person with even
>> the most basic skill set?
>>
>> IMHO a docstring for private_cccache would add some real value, but this
>> comment alone does not.
>>
>>>
>>> For a newcomer to the project, even a trivial comment (from the point of
>>> view of the experienced developer) can be valuable.
>>
>> Following this logic, there should be a comment for every line of our
>> code, which is ridiculous.
>>
>
> Here's the version of the patch with the comment removed.
IMHO the comment should have been something like "ensure no KRB5CCNAME
otherwise it blows up in ..." If it took you 20 minutes to track down a
one-line change then a comment might save the next guy who changes the
behavior.
rob
--
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
