Bug 33125 - Use OptionParser instead of getopt in test-webkit-script
Summary: Use OptionParser instead of getopt in test-webkit-script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-03 15:01 PST by Chris Jerdonek
Modified: 2010-01-05 11:24 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 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)
""" :)
Comment 1 Chris Jerdonek 2010-01-04 21:39:53 PST
Created attachment 45861 [details]
Proposed patch
Comment 2 Eric Seidel (no email) 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__)?
Comment 3 WebKit Review Bot 2010-01-04 21:57:43 PST
style-queue ran check-webkit-style on attachment 45861 [details] without any errors.
Comment 4 Chris Jerdonek 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!
Comment 5 Chris Jerdonek 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.
Comment 6 Chris Jerdonek 2010-01-04 23:22:43 PST
Created attachment 45864 [details]
Proposed patch 2
Comment 7 WebKit Review Bot 2010-01-04 23:26:43 PST
style-queue ran check-webkit-style on attachment 45864 [details] without any errors.
Comment 8 Eric Seidel (no email) 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.
Comment 9 Chris Jerdonek 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)?
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2010-01-05 00:47:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Eric Seidel (no email) 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.