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) """ :)
Created attachment 45861 [details] Proposed patch
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__)?
style-queue ran check-webkit-style on attachment 45861 [details] without any errors.
(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!
(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.
Created attachment 45864 [details] Proposed patch 2
style-queue ran check-webkit-style on attachment 45864 [details] without any errors.
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.
(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 on attachment 45864 [details] Proposed patch 2 Clearing flags on attachment: 45864 Committed r52790: <http://trac.webkit.org/changeset/52790>
All reviewed patches have been landed. Closing bug.
I think Adam may have won the "/' argument by sheer force of will, or possible bribery with cake.