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.
killed
>
>  - 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"]
Fixed to use a deepcopy instead of shallow
>
> 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]
+ #abuse the order of lookuplist in order to have lookups done in the
correct order, "env" first.
>
> 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.
>
In the old code configlist starts with env and ends with globals, but
the lookuplist is configlist.reverse()

<oldcode>
self.configdict = { "globals":   self.configlist[0],
                    "defaults":  self.configlist[1],
                    "conf":      self.configlist[2],
                    "pkg":       self.configlist[3],
                    "auto":      self.configlist[4],
                    "backupenv": self.configlist[5],
                    "env":       self.configlist[6] }

                        # make lookuplist for loading package.*
                        self.lookuplist=self.configlist[:]
                        self.lookuplist.reverse()
</oldcode>

> --
> Jason Stubbs
>