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

[Alec Warner]

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
>