Bug 39208 - webkit-patch needs --verbose flag to enable DEBUG logging
Summary: webkit-patch needs --verbose flag to enable DEBUG logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-17 01:37 PDT by Eric Seidel (no email)
Modified: 2010-05-24 01:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.86 KB, patch)
2010-05-17 01:40 PDT, Eric Seidel (no email)
cjerdonek: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-05-17 01:37:25 PDT
webkit-patch needs --verbose flag to enable DEBUG logging
Comment 1 Eric Seidel (no email) 2010-05-17 01:40:13 PDT
Created attachment 56222 [details]
Patch
Comment 2 Chris Jerdonek 2010-05-17 02:09:02 PDT
Comment on attachment 56222 [details]
Patch

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.
Comment 3 Eric Seidel (no email) 2010-05-24 01:27:41 PDT
Addressed Chris's comments.  landing.
Comment 4 Eric Seidel (no email) 2010-05-24 01:32:39 PDT
Committed r60066: <http://trac.webkit.org/changeset/60066>