Fix watchlists for windows, for files with backslash

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18764

[M-A Ruel, 2009-06-18 18:05:28]

http://codereview.chromium.org/132022/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/132022/diff/1/3#newcode113
Line 113: path = path.replace('\\', '/')
path = path.replace(os.sep, '/')

Works every time. No need to check for the platform.

[Nirnimesh, 2009-06-18 18:10:22]

http://codereview.chromium.org/132022/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/132022/diff/1/3#newcode113
Line 113: path = path.replace('\\', '/')
On 2009/06/18 18:05:28, Marc-Antoine Ruel wrote:
> path = path.replace(os.sep, '/')
> 
> Works every time. No need to check for the platform.

But then how do I write the unit-test? I'd like to be able to run this test from
posix too.

[M-A Ruel, 2009-06-18 18:13:31]

On 2009/06/18 18:10:22, Nirnimesh wrote:
> http://codereview.chromium.org/132022/diff/1/3
> File watchlists.py (right):
> 
> http://codereview.chromium.org/132022/diff/1/3#newcode113
> Line 113: path = path.replace('\\', '/')
> On 2009/06/18 18:05:28, Marc-Antoine Ruel wrote:
> > path = path.replace(os.sep, '/')
> > 
> > Works every time. No need to check for the platform.
> 
> But then how do I write the unit-test? I'd like to be able to run this test
from
> posix too.

I never do that. I never flavor complexifying the source code to make it more
testable, specifically for simple changes like that. But I don't mind.

[John Grabowski, 2009-06-18 18:21:25]

LGTM with minor tweaks

http://codereview.chromium.org/132022/diff/1/2
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/132022/diff/1/2#newcode130
Line 130: def testWinPathWatchers(self):
A unit test!  I wuv u!

http://codereview.chromium.org/132022/diff/1/2#newcode152
Line 152: ['chrome\\browser\\renderer_host\\render_widget_host.h']), watchers)
Use this construct for "raw" strings so you don't need to double the backslash:
  r'rawstring'
e.g.
r'chrome\browser\blah'

http://codereview.chromium.org/132022/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/132022/diff/1/3#newcode113
Line 113: path = path.replace('\\', '/')
On 2009/06/18 18:10:22, Nirnimesh wrote:
> On 2009/06/18 18:05:28, Marc-Antoine Ruel wrote:
> > path = path.replace(os.sep, '/')
> > 
> > Works every time. No need to check for the platform.
> 
> But then how do I write the unit-test? I'd like to be able to run this test
from
> posix too.

Unlike maruel, I'd be quite happy to add a little complexity to improve
testability.  However, in this case os.sep is a good idea.  For unit tests,
simply redefine os.sep to be what you need.  E.g.
  os.sep = r'\'
(Yes it's that easy)

[Nirnimesh, 2009-06-18 18:22:31]

I think this test is important to have. Yesterday darin discovered that
watchlists for windows wasn't working because the the backslash thingy.

[Nirnimesh, 2009-06-18 18:40:43]

http://codereview.chromium.org/132022/diff/1/2
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/132022/diff/1/2#newcode152
Line 152: ['chrome\\browser\\renderer_host\\render_widget_host.h']), watchers)
On 2009/06/18 18:21:25, John Grabowski wrote:
> Use this construct for "raw" strings so you don't need to double the
backslash:
>   r'rawstring'
> e.g.
> r'chrome\browser\blah'
> 

Done.

http://codereview.chromium.org/132022/diff/1/3
File watchlists.py (right):

http://codereview.chromium.org/132022/diff/1/3#newcode113
Line 113: path = path.replace('\\', '/')
On 2009/06/18 18:05:28, Marc-Antoine Ruel wrote:
> path = path.replace(os.sep, '/')
> 
> Works every time. No need to check for the platform.

Done.

http://codereview.chromium.org/132022/diff/1/3#newcode113
Line 113: path = path.replace('\\', '/')
On 2009/06/18 18:21:25, John Grabowski wrote:
> On 2009/06/18 18:10:22, Nirnimesh wrote:
> > On 2009/06/18 18:05:28, Marc-Antoine Ruel wrote:
> > > path = path.replace(os.sep, '/')
> > > 
> > > Works every time. No need to check for the platform.
> > 
> > But then how do I write the unit-test? I'd like to be able to run this test
> from
> > posix too.
> 
> Unlike maruel, I'd be quite happy to add a little complexity to improve
> testability.  However, in this case os.sep is a good idea.  For unit tests,
> simply redefine os.sep to be what you need.  E.g.
>   os.sep = r'\'
> (Yes it's that easy)
> 

Hmm.. I was worried that other tests would see a changed os.sep.
Anyways, I've saved a copy of os.sep and reverted back after this test.

Thanks