-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start of issue 718, file not existing clean up #772
Conversation
|
Can 2 people please review this code? @b-wyss @bocajnotnef @brtaylor92 |
Looks good to me minus the non-mergable status. |
Looks good to me as well, unless you want to change existance to existence? Totally up to you, but the rest looks okay! |
mode = os.stat(file_path).st_mode | ||
try: | ||
mode = os.stat(file_path).st_mode | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary empty line ;)
fn = utils.get_temp_filename('thisfiledoesnotexist') | ||
try: | ||
check_file_status(fn, False) | ||
assert True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ctb could you remind me of the idea behind the if/if not statement to be put here you mentioned today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could rewrite it as:
check_file_status_exited = False
try:
check_file_status(fn, False)
except SystemExit:
check_file_status_exited = True
assert check_file_status_exited
which makes the logic clear, at least.
0155591
to
d0cc0b0
Compare
@mr-c has the compatibility problem with pep8/autopep8 been fixed? |
Yes, sorry. Update to the latest from the master branch and you should be On Fri, Mar 13, 2015, 14:38 jessicamizzi notifications@github.com wrote:
|
Conflicts: ChangeLog
Start of issue 718, file not existing clean up
Thanks! |
Fix for #718.