[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]

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.