Fixing cygwin specific assumptions in gyp diff writer.

Fixing cygwin specific assumptions in gyp diff writer.


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

[TVL, 2009-03-20 18:01:03]

drive by

http://codereview.chromium.org/49004/diff/1/2
File pylib/gyp/common.py (right):

http://codereview.chromium.org/49004/diff/1/2#newcode262
Line 262: # existing file, so it must be preceded with a rename. Sadly there is
s/rename/remove/

[Mark Mentovai, 2009-03-20 18:27:39]

http://codereview.chromium.org/49004/diff/1/2
File pylib/gyp/common.py (right):

http://codereview.chromium.org/49004/diff/1/2#newcode267
Line 267: if sys.platform == 'win32': os.remove(filename)
I'd prefer to see os.remove on its own indented line.

The comment about why we're doing the remove on Windows should also be inside
the "if" right before os.remove, where it's more likely to be noticed, instead
of in a separate "NOTE:" above.

[bradn, 2009-03-20 20:39:00]

Thanks all.
Have another look.

http://codereview.chromium.org/49004/diff/1/2
File pylib/gyp/common.py (right):

http://codereview.chromium.org/49004/diff/1/2#newcode262
Line 262: # existing file, so it must be preceded with a rename. Sadly there is
On 2009/03/20 18:01:03, TVL wrote:
> s/rename/remove/

Done.

http://codereview.chromium.org/49004/diff/1/2#newcode267
Line 267: if sys.platform == 'win32': os.remove(filename)
On 2009/03/20 18:27:39, Mark Mentovai wrote:
> I'd prefer to see os.remove on its own indented line.
> 
> The comment about why we're doing the remove on Windows should also be inside
> the "if" right before os.remove, where it's more likely to be noticed, instead
> of in a separate "NOTE:" above.

Done.

[Mark Mentovai, 2009-03-20 20:40:51]

Great, LGTM.  Let sgk know when you check this in, I think he's waiting on it.