Danek Duvall wrote:
> On Thu, Dec 11, 2008 at 09:48:28AM -0800, Rich Burridge wrote:
>
>> http://cr.opensolaris.org/~richb/pkg-5705-v1/
>
> 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.
Is there a disadvantage to using classes or a standard I should
be following here?
>
> line 58: use "latest" instead of "101a" as the default.
Okay.
>
> line 61, 65, 69, 73: drop
So changed.
>
> line 62, 66, 70: things might be faster if these were sets instead of
> lists.
Here's the timing for three consecutive runs on my Ultra 40:
$ time python check_install_scripts.py >/tmp/list.txt
real 0m6.624s
user 0m0.823s
sys 0m0.514s
$ time python check_install_scripts.py >/tmp/list.txt
real 0m2.540s
user 0m0.819s
sys 0m0.333s
$ 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? :-)
> line 87 (et al): We've been using "print >> sys.stderr, ..." elsewhere.
So changed.
>
> line 90: this variable is never read from.
Good catch. I think this is a relic from originally starting from the
check_depends.py script (in bug #5268). I've removed this line
(and the imports.append one at line 120). If I'm missing something here
Albert, please let me know.
>
> line 95, 110: you use two different mechanisms to do the same thing. Be
> consistent.
Okay. I changed the second one to be like the first.
>
> 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):
?
Note that I was copying the code in the usage() function in
.../gate/src/client.py. It's similar in .../gate/src/pull.py.
>
> line 156: Ew. Use os.listdir().
Okay.
>
> line 160: Either use parens or use continuation characters, not both.
> We've been using continuation characters elsewhere.
Okay.
>
> line 193: I'd use "%-28s" here instead of ljust(), as well as for the
> header lines.
So changed.
>
> 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.
> 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.
> In general, you don't handle "drop", "exclude", or "from", and read in way
> too many import files, so the utility of this script seems quite limited.
> If David thinks this will be useful, go ahead and integrate it, but it's
> not at all clear to me that this approach will actually reduce the amount
> of work needed to keep the import files up to date. I'd be more inclined
> to believe that something checking for new and modified {pre,post}install
> scripts from one build to the next would be far more valuable. But again,
> this is David's call.
Noted.
New timings for the three runs is now:
$ time python check_install_scripts.py >/tmp/list.txt
real 0m5.176s
user 0m0.816s
sys 0m0.510s
$ time python check_install_scripts.py >/tmp/list.txt
real 0m3.120s
user 0m0.815s
sys 0m0.359s
$ time python check_install_scripts.py >/tmp/list.txt
real 0m1.874s
user 0m0.808s
sys 0m0.274s
New webrev at:
http://cr.opensolaris.org/~richb/pkg-5705-v2/
Thanks.
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
