Summary

Fixed RepositoryTool patching of files in uncreated directories

Review Request #10247 — Created Oct. 19, 2018 and submitted Nov. 10, 2018, 11:26 a.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

Commit

8d24901...

Reviewers

Groups

reviewbot, students

People

Description

When a RepositoryTool is applying the review's patches to the cloned
repository, it simply writes using open(p, 'wb'). This does not create
directories that the patch implies.

This leads to the write failing with:
IOError: [Errno 2] No such file or directory: u'dir/file'

This patch creates directories for each file if the path does not exist
already.

Testing Done

Replciated the error by posting a review for a file in a new directory,
applied patch, and RepositoryTool successfully runs on patched files.

Issues

Description	From	Last Updated
F401 'os.path' imported but unused	reviewbot	Oct. 19, 2018, 5:38 p.m.
F811 redefinition of unused 'path' from line 8	reviewbot	Oct. 19, 2018, 5:45 p.m.
E722 do not use bare except'	reviewbot	Oct. 19, 2018, 5:45 p.m.
Blank line between these.	brennie	Oct. 25, 2018, 3:43 p.m.
This doesn't always create directories -- it ensures they exist, so let's call this ensure_directories.	brennie	Oct. 25, 2018, 4:08 p.m.
No space after "	brennie	Oct. 25, 2018, 3:45 p.m.
The path for which directories should be created if they don't exist.	brennie	Oct. 25, 2018, 3:45 p.m.
Blank line between here.	brennie	Oct. 25, 2018, 3:45 p.m.
E722 do not use bare except'	reviewbot	Oct. 19, 2018, 5:46 p.m.
We never want to use bare except. This can catch things that should never be caught, like KeyboardInterrupt or RuntimeError. …	brennie	Oct. 25, 2018, 4:08 p.m.
It seems like we'd probably want to surface the error so that the caller can bail out.	david	Oct. 30, 2018, 5:10 p.m.
Function name would be better as ensure_directories_exist	ammar	Oct. 31, 2018, 3:30 p.m.

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/utils/filesystem.py (Diff revision 1)
Show all issues
```
F401 'os.path' imported but unused
```
bot/reviewbot/utils/filesystem.py (Diff revision 1)
Show all issues
```
F811 redefinition of unused 'path' from line 8
```
bot/reviewbot/utils/filesystem.py (Diff revision 1)
Show all issues
```
E722 do not use bare except'
```

Change Summary:

Removed unneccessary import

Commit:

-	c89f67ab14f7da66647f24441492effb299a19cd
+	68b16e6d2c54cd6f564379b0aced9d654dadacd3

Diff:

Revision 2 (+17 -1)

Show changes

	bot/reviewbot/tools/__init__.py
	bot/reviewbot/utils/filesystem.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

bot/reviewbot/utils/filesystem.py (Diff revision 2)
Show all issues
```
E722 do not use bare except'
```

bot/reviewbot/tools/__init__.py (Diff revision 2)
Show all issues
```
Blank line between these.
```

bot/reviewbot/utils/filesystem.py (Diff revision 2)

Show all issues

This doesn't always create directories -- it ensures they exist, so let's call this ensure_directories.

bot/reviewbot/utils/filesystem.py (Diff revision 2)
Show all issues
```
No space after "
```

bot/reviewbot/utils/filesystem.py (Diff revision 2)

Show all issues

The path for which directories should be created if they don't exist.

bot/reviewbot/utils/filesystem.py (Diff revision 2)
Show all issues
```
Blank line between here.
```

bot/reviewbot/utils/filesystem.py (Diff revision 2)

Show all issues

We never want to use bare except. This can catch things that should never be caught, like KeyboardInterrupt or RuntimeError. Instead, this needs to specify the exact exception we expect, which should be OSError.

alextechcc Oct. 23, 2018, 7:38 p.m.

Should I fix the rest of the instances of this in all related files?

brennie Oct. 23, 2018, 8:24 p.m.
```
No, that isn't related to your change.
```