Summary: | webkit-patch needs --verbose flag to enable DEBUG logging | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cjerdonek, ojan | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2010-05-17 01:37:25 PDT
Created attachment 56222 [details]
Patch
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. Addressed Chris's comments. landing. Committed r60066: <http://trac.webkit.org/changeset/60066> |