WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
33125
Use OptionParser instead of getopt in test-webkit-script
https://bugs.webkit.org/show_bug.cgi?id=33125
Summary
Use OptionParser instead of getopt in test-webkit-script
Chris Jerdonek
Reported
2010-01-03 15:01:18 PST
Eric suggested this and some other improvements in
bug 33045, comment 16
: """A few python comments: 1. Should really use a class to encapsulate the logic. Then your __main__ just calls the class Foo().main() 2. OptionParser (from optparse) is probably easier to use than getopt. I would have probably made: os.path.join(scripts_directory, 'test-webkit-perl') a function, even though you only use it twice. def script_path(script): return os.path.join(scripts_directory, script) """ :)
Attachments
Proposed patch
(5.13 KB, patch)
2010-01-04 21:39 PST
,
Chris Jerdonek
no flags
Details
Formatted Diff
Diff
Proposed patch 2
(5.09 KB, patch)
2010-01-04 23:22 PST
,
Chris Jerdonek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Chris Jerdonek
Comment 1
2010-01-04 21:39:53 PST
Created
attachment 45861
[details]
Proposed patch
Eric Seidel (no email)
Comment 2
2010-01-04 21:44:43 PST
Comment on
attachment 45861
[details]
Proposed patch 60 if args is not None: can just be "if args", no? 85 tester = ScriptsTester(sys.path[0]) doens't seem right. Wouldn't you want to use os.path.dirname(__file__)?
WebKit Review Bot
Comment 3
2010-01-04 21:57:43 PST
style-queue ran check-webkit-style on
attachment 45861
[details]
without any errors.
Chris Jerdonek
Comment 4
2010-01-04 22:13:40 PST
(In reply to
comment #2
) Thanks, Eric.
> (From update of
attachment 45861
[details]
) > 60 if args is not None: > > can just be "if args", no?
From the Python style guide, for instance:
> Also, beware of writing "if x" when you really mean "if x is not None" > -- e.g. when testing whether a variable or argument that defaults to > None was set to some other value. The other value might have a type > (such as a container) that could be false in a boolean context!
(
http://www.python.org/dev/peps/pep-0008/
under "Programming Recommendations") It's also done this way in the section on default argument values in the Google Python style guide:
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Default_Argument_Values
> > 85 tester = ScriptsTester(sys.path[0]) > > doens't seem right. > > Wouldn't you want to use os.path.dirname(__file__)?
Yeah, I think so. It looks like what I wrote won't work if test-webkit-scripts is invoked by another script outside of the Scripts directory. Thanks!
Chris Jerdonek
Comment 5
2010-01-04 22:51:43 PST
(In reply to
comment #4
)
> > (From update of
attachment 45861
[details]
[details]) > > 60 if args is not None: > > > > can just be "if args", no? > > From the Python style guide, for instance:
Never mind, yours is better. I'm not really setting a default argument. And no need to call extend() on an empty list that was passed.
Chris Jerdonek
Comment 6
2010-01-04 23:22:43 PST
Created
attachment 45864
[details]
Proposed patch 2
WebKit Review Bot
Comment 7
2010-01-04 23:26:43 PST
style-queue ran check-webkit-style on
attachment 45864
[details]
without any errors.
Eric Seidel (no email)
Comment 8
2010-01-05 00:03:41 PST
Comment on
attachment 45864
[details]
Proposed patch 2 Adam convinced me to standardize on " instead of ' (even though I personally prefer '), I think this is fine as-is, but in the future you should consider using ". As far as I can tell the python style guide expresses no preference, but using one over the other makes search/replace slightly easier. LGTM.
Chris Jerdonek
Comment 9
2010-01-05 00:24:45 PST
(In reply to
comment #8
)
> (From update of
attachment 45864
[details]
) > Adam convinced me to standardize on " instead of ' (even though I personally > prefer '), I think this is fine as-is, but in the future you should consider > using ". As far as I can tell the python style guide expresses no preference, > but using one over the other makes search/replace slightly easier.
My first exposure to WebKit Python was check-webkit-style, which uses ' exclusively, so I was simply continuing with that. Without having heard any arguments, I think I prefer " because it's consistent with docstring quotes """ (which the style guide does say should be double quotes). What were Adam's arguments for double, and what was the reason for your preference to use single (and why were you convinced)?
WebKit Commit Bot
Comment 10
2010-01-05 00:47:41 PST
Comment on
attachment 45864
[details]
Proposed patch 2 Clearing flags on attachment: 45864 Committed
r52790
: <
http://trac.webkit.org/changeset/52790
>
WebKit Commit Bot
Comment 11
2010-01-05 00:47:48 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 12
2010-01-05 11:24:53 PST
I think Adam may have won the "/' argument by sheer force of will, or possible bribery with cake.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug