Bug 33125 – Use OptionParser instead of getopt in test-webkit-script

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Jerdonek]

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)
""" :)

[Chris Jerdonek]

Created attachment 45861 [details]
Proposed patch

[Eric Seidel (no email)]

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]

style-queue ran check-webkit-style on attachment 45861 [details] without any errors.

[Chris Jerdonek]

(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]

(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]

Created attachment 45864 [details]
Proposed patch 2

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45864 [details] without any errors.

[Eric Seidel (no email)]

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]

(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 on attachment 45864 [details]
Proposed patch 2

Clearing flags on attachment: 45864

Committed r52790: <http://trac.webkit.org/changeset/52790>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I think Adam may have won the "/' argument by sheer force of will, or possible bribery with cake.