Skip to content

Commit

Permalink
Use 'del' and 'rmdir' on Windows
Browse files Browse the repository at this point in the history
  • Loading branch information
jacebrowning committed Jan 8, 2017
1 parent 811dddf commit 7e574ab
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 16 deletions.
13 changes: 7 additions & 6 deletions gitman/shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,13 @@ def ln(source, target):


def rm(path):
show('rm', '-rf', path)
if os.path.exists(path):
if os.path.isdir(path):
shutil.rmtree(path)
else:
os.remove(path)
if os.name == 'nt':
if os.path.isfile(path):
call('del', '/Q', '/F', path, _shell=True)
elif os.path.isdir(path):
call('rmdir', '/Q', '/S', path, _shell=True)
else:
call('rm', '-rf', path)


def show(name, *args, stdout=True):
Expand Down
21 changes: 11 additions & 10 deletions gitman/tests/test_shell.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,20 @@ def test_ln_missing_parent(self, mock_call):
shell.ln('mock/target', 'mock/source')
assert_calls(mock_call, ["ln -s mock/target mock/source"])

@patch('os.remove')
@patch('os.path.exists', Mock(return_value=True))
def test_rm_file(self, mock_remove, mock_call):
@patch('os.path.isfile', Mock(return_value=True))
def test_rm_file(self, mock_call):
"""Verify the commands to delete files."""
shell.rm('mock/path')
mock_remove.assert_called_once_with('mock/path')
assert_calls(mock_call, [])
if os.name == 'nt':
assert_calls(mock_call, ["del /Q /F mock/path"])
else:
assert_calls(mock_call, ["rm -rf mock/path"])

@patch('shutil.rmtree')
@patch('os.path.exists', Mock(return_value=True))
@patch('os.path.isdir', Mock(return_value=True))
def test_rm_directory(self, mock_rmtree, mock_call):
def test_rm_directory(self, mock_call):
"""Verify the commands to delete directories."""
shell.rm('mock/dirpath')
mock_rmtree.assert_called_once_with('mock/dirpath')
assert_calls(mock_call, [])
if os.name == 'nt':
assert_calls(mock_call, ["rmdir /Q /S mock/dirpath"])
else:
assert_calls(mock_call, ["rm -rf mock/dirpath"])

2 comments on commit 7e574ab

@StudioEtrange
Copy link
Contributor

@StudioEtrange StudioEtrange commented on 7e574ab Jan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacebrowning
in shell.rm()
you should keep if os.path.exists(path): at least for windows.
Because rmdir /Q /S throw an error of path does not exist

@StudioEtrange
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacebrowning
Ok so you pick code piece by piece. And try to fix some things or arrange some things at the same time.
I was thinking you will pick several bag of commits as is.

Are you really sure you dont want juste merge the PR, and fix after that the one or two things that you dislike ? There is a lot of chance that you will re-do the bug I have fixed while I was working on this PR and windows support.

Just merge the PR, and change things after :)

Please sign in to comment.