Jason Stubbs wrote: > On Saturday 25 February 2006 10:40, Alec Warner wrote: > >>Please review for badness, otherwise I'll commit this soon ;) > > > - self.backupenv = os.environ.copy() > - self.configlist.append(self.backupenv) # XXX Why though? > - self.configdict["backupenv"]=self.configlist[-1] > + self.configdict["backupenv"]=os.environ.copy() # XXX Why though? ( > ferringb? ) > > Kill this comment altogether. Reading it, my first thought is "why what?" > It doesn't make the code clearer and so shouldn't be there. > > - self.configlist.append(os.environ.copy()) > - self.configdict["env"]=self.configlist[-1] > + ### backupenv maybe required to be a clone, and not just a reference > + ### the old code did value copy we do a reference here > + self.configdict["env"]=self.configdict["backupenv"] > > Again, this comment doesn't make the code clearer and so shouldn't be there. > Changing functionality in a "cleanup" patch is also bad form. As for the > change itself, it's broken. Look at __setitem__() and reset() and then look > at the usage of those two throughout the code. > > - mydbs=self.configlist[:-1] > + #Abuse the order of lookuplist to emulate old behavior > + mydbs=self.lookuplist[:-1] > > Same deal with this comment. A person seeing the code for the first time has > no idea what the old behaviour was. In the old code, configlist starts with > "globals" and ends with "env". In the new (and old) code, lookuplist is the > reverse of that. I don't see any further changes to account for this. > > - if 0 and match and mykey in ["PORTAGE_BINHOST"]: > - # These require HTTP Encoding > ... > > This shouldn't be in a "cleanup" patch either. >
Er, may I ask why it's in the code at all? :) > -- > Jason Stubbs >
signature.asc
Description: OpenPGP digital signature
