Re: [gentoo-portage-dev] Config Cleanup Last Call

[Jason Stubbs]

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.

--
Jason Stubbs

-- 
gentoo-portage-dev@g.o mailing list