[webkit-reviews] review denied: [Bug 35498] check-webkit-style: Path-specific logic should work when invoking with files syntax : [Attachment 51482] Proposed patch 7

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 24 05:03:11 PDT 2010


Shinichiro Hamaji <hamaji at chromium.org> has denied Chris Jerdonek
<cjerdonek at webkit.org>'s request for review:
Bug 35498: check-webkit-style: Path-specific logic should work when invoking
with files syntax
https://bugs.webkit.org/show_bug.cgi?id=35498

Attachment 51482: Proposed patch 7
https://bugs.webkit.org/attachment.cgi?id=51482&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
As usual, this patch looks very good :) r- for a few style nitpicks.

> +	   _log.warn("WebKit checkout root not found:\n"
> +		     "	Path-dependent style checks may not work correctly.\n"
> +		     "	See the help documentation for more info.")
> +
> +	   return paths
> +
> +    if paths:
> +	   # Then try converting all of the paths to paths relative to
> +	   # the checkout root.
> +	   rel_paths = []
> +	   for path in paths:
> +	       rel_path = _rel_path(path, checkout_root)
> +	       if rel_path is None:
> +		   # Then the path is not below the checkout root.  Since all
> +		   # paths should be interpreted relative to the same root,
> +		   # do not interpret any of the paths as relative to the
> +		   # checkout root.  Interpret all of them relative to the
> +		   # current working directory, and do not change the current
> +		   # working directory.
> +		   _log.warn("\
> +Path-dependent style checks may not work correctly:\n\
> +\n\
> +  One of the given paths is outside the WebKit checkout of the current\n\
> +  working directory:\n\
> +\n\
> +    Path: %s\n\
> +    Checkout root: %s\n\
> +\n\
> +  Pass only files below the checkout root to ensure correct results.\n\
> +  See the help documentation for more info.\n"
> +			     % (path, checkout_root))

I'm not sure why we are using this style here. I prefer the style of above
_log.error. Or, we may want to use a string literal with three double-quotes?

> +	   expected_rel_path = os.path.join("test", "Foo.txt")
> +	   path = os.path.join(start_path, expected_rel_path)

I'd create these values by ["test", "Foo.txt"].join("/") so the test case
becomes OS independent. Of course, we may want one test case which uses
backslashes as the path separator.

Also, I'd check rel_path("WebKit//test//Foo.txt", "WebKit") == "test//Foo.txt"
.

> +	   actual_rel_path = self._rel_path(path, start_path)
> +	   self.assertEquals(expected_rel_path, actual_rel_path)

I'd remove actual_rel_path, but it's OK as is.

> +	   rel_path = self._rel_path(path, start_path)
> +	   self.assertTrue(rel_path is None)
> +
> +	   rel_path = self._rel_path("WebKitTools", "WebKit")
> +	   self.assertTrue(rel_path is None)

Same here.

> +	   log_messages = ["""WARNING: WebKit checkout root not found:
> +  Path-dependent style checks may not work correctly.
> +  See the help documentation for more info.
> +"""]

> +	   log_messages = [
> +"""WARNING: Path-dependent style checks may not work correctly:
> +
> +  One of the given paths is outside the WebKit checkout of the current
> +  working directory:
> +
> +    Path: /outside/foo2.txt
> +    Checkout root: /WebKit
> +
> +  Pass only files below the checkout root to ensure correct results.
> +  See the help documentation for more info.
> +
> +"""]

They look inconsistent, too.

> +	   if not found_checkout and not paths:
> +	       _log.error("WebKit checkout not found: You must run this script
"
> +			  "from inside a WebKit checkout if you are not passing
"
> +			  "specific paths to check.")
> +	       sys.exit(1)

I'd put this in check-webkit-style and then move it to main.py when we remove
FIXME in check-webkit-style you added in this patch. But it's not strong
preference and please feel free to go ahead as is.

>      def _parse(self):
>	   """Return a default parse() function for testing."""
> -	   return self._create_parser().parse
> +	   # We will test found_scm False separately.
> +	   def parse(args, found_scm=True):
> +	       parser = self._create_parser()
> +	       return parser.parse(args, found_scm)
> +
> +	   return parse

I didn't notice this function is a higher order function when I looked this
before, but I'd just define this like

def _parse(self, args, found_scm=True):
    """..."""
    parser = self._create_parser()
    return parser.parse(args, found_scm)

and use it like

parse = self._parse
self.assertRaises(SystemExit, parse, [...])

because this would be simpler.


More information about the webkit-reviews mailing list