[webkit-reviews] review granted: [Bug 39208] webkit-patch needs --verbose flag to enable DEBUG logging : [Attachment 56222] Patch

[bugzilla-daemon at webkit.org]

Chris Jerdonek <cjerdonek at webkit.org> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 39208: webkit-patch needs --verbose flag to enable DEBUG logging
https://bugs.webkit.org/show_bug.cgi?id=39208

Attachment 56222: Patch
https://bugs.webkit.org/attachment.cgi?id=56222&action=review

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Some thoughts to consider prior to landing.  r=me

> +    is_verbose = "-v" in sys.argv or "--verbose" in sys.argv

I wonder if there is a nice way to avoid writing sys.argv twice -- e.g.
Python set intersection?

> +    def _command_for_printing(self, args):
> +	   """Used for producing a print-ready string representing command
args.
> +	   The string should be copy/paste ready for execution in a shell."""

PEP8 says the doc string for functions should begin with an imperative
word, e.g. "Return a print-ready string...."

PEP8 also has pretty specific rules about the formatting of doc strings.  This
one would look something like this--

    def _command_for_printing(self, args):
	"""Return a print-ready string that represents command args.

	The string should be copy/paste ready for execution in a shell.
	
	"""

> +	       if isinstance(arg, unicode):
> +		   # Escape any non-ascii characters for easy copy/paste
> +		   arg = arg.encode("unicode_escape")
> +	       # quotes?

What does the "quotes?" comment mean -- is that a FIXME?

>  
> +	   _log.debug("\"%s\" took %.2fs" % (self._command_for_printing(args),
time.time() - start_time))

I would use single quotes to avoid escaping the double quotes.