On 23.11.2015 15:19, Rob Crittenden wrote:
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.
As I wrote earlier, I think it would make more sense to put this into
the docstring of private_ccache().
ACK on the patch.
--
Jan Cholasta
--
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
