[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