On 11/25/2015 09:04 AM, Jan Cholasta wrote: > 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. >
Pushed to master: 1904d7cc3ab33046428dbdcb7c6f521f9e083287 -- 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