[webkit-reviews] review granted: [Bug 35498] check-webkit-style: Path-specific logic should work when invoking with files syntax : [Attachment 51837] Proposed patch 8
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Mar 27 19:56:09 PDT 2010
Shinichiro Hamaji <hamaji at chromium.org> has granted 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 51837: Proposed patch 8
https://bugs.webkit.org/attachment.cgi?id=51837&action=review
------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
> > > + 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.
>
> I could be mistaken on the comments I'm making here, but isn't it preferable
if
> we can eliminate the presence of forward and backward slashes from the tests
> (i.e. eliminate platform-specific path separators and generate all paths
using
> cross-platform methods)? One reason, for example, is that os.path.abspath()
> (which _rel_path() uses) won't work for paths with backward slashes when run
> from a Mac:
>
> http://docs.python.org/library/os.path.html#os.path.abspath
> http://docs.python.org/library/os.path.html#os.path.normpath
>
> I guess it wouldn't matter for our tests if we were using mock os.path
methods
> for all of our path processing (which we aren't really). But if we were,
then
> it doesn't seem like it would help to test both forward and backward slashes,
> etc. since then we would just be testing the mock methods rather than the
code
> itself.
>
> I think my preference is to use os.path methods everywhere possible rather
than
> literal slashes. Otherwise, it seems like we would have to explicitly
import,
> for example, ntpath to test the case of backward slashes.
I see. I didn't notice os.path.abspath depends on os.path.sep. So, now I think
using os.path.join would be better. As you are using a slash and a backslash in
_rel_path to lstrip the path, I thought testing without os.path.join would be
better to check both slashes and backslashes will be lstripped.
> > > + 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.
>
> Thanks, you are right. That was left over from previous.
>
> >
> > > + 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.
>
> ?
Sorry for this unclear comment. I tried to mean we can remove actual_rel_path
and rel_path. Like
self.assertEquals(expected_rel_path, self._rel_path(path, start_path))
or
self.assertTrue(self._rel_path(path, start_path) is None)
But this change isn't important at all. Please go ahead as is if you like the
current code.
> FYI, this difference turned out to have been forced by the second case
> exceeding the 79 character limit (my preference was to start immediately
after
> the bracket by default, if possible). Maybe the solution is to always begin
> triple double-quote strings at the beginning of the following line, when used
> in cases other than doc strings.
If you like, you may be able to use
str = """\
Foo
bar
baz
"""
but your code is good as well as is.
More information about the webkit-reviews
mailing list