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.

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

Reply via email to