On Thu, Dec 11, 2008 at 01:38:50PM -0800, Rich Burridge wrote: >> Why are you using a class? > > Because I prefer to use self.<variable> to either using globals > or passing around a load of arguments to each method/function. > Personally I also think it makes the code look cleaner.
Okay. Since this class is the only thing in the script, it seems that you could use the script as the method of encapsulation and save yourself a level of indentation, but I don't care too strongly. >> line 62, 66, 70: things might be faster if these were sets instead of >> lists. > Here's the timing [ ... ] on my Ultra 40: > > $ time python check_install_scripts.py >/tmp/list.txt > > real 0m2.217s > user 0m0.819s > sys 0m0.273s > > I think that's fast enough isn't it? :-) Sure, but that's no excuse to get into the habit of using data structures which are slow for a particular purpose. >> line 123: no spaces around equals in argument lists. > > Sorry, not sure I understand. You want: > > def usage(self, usage_error = None): > > to be: > > def usage(self, usage_error=None): > > ? Yes. > Note that I was copying the code in the usage() function in > .../gate/src/client.py. It's similar in .../gate/src/pull.py. Yes. I started off by putting spaces around the equals, but after being beaten over the head with PEP8, decided that we should switch to that for the standard. We've been slowly removing the spaces as we change the surrounding code, and making sure that new code follows the standard. >> line 239: Ew. One: just use os.walk(). > > If I'm reading the manual pages correctly, I'd have to call > os.path.walk(self.prefix, myfunc, None) > then each time myfunc is called, I'd have to call os.listdir() > to get a list of all the files in that directory, and for each one > I'd then have to see if it's of type "f" (os.path.isfile()). > > Or I could just call: > > cmd = 'find %s -type f -print' % self.prefix > lines = os.popen(cmd).readlines() > > and let find do all the work for me. I'm not a fan of shelling out to external programs except in shell scripts, particularly when the utility exists natively. But I'm not going to press the point. >> Two: this finds all files under the current directory, regardless of >> what build they belong to, or even whether they're a descriptor file >> that the importer would ever read! That sounds like it'll lead to a >> hell of a lot of useless noise. > > Yes, but it doesn't take long and the code catches (and ignores) all the > files it isn't interested in. How, exactly? It complains if there isn't a "package" statement in a given file, but that doesn't help when you're looking at an import file that's not used for the build you're checking. Danek _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
