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

Reply via email to