-
Notifications
You must be signed in to change notification settings - Fork 49
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
Correct index.lock file deletion and other fixes & formatting #52
Conversation
@sebdah Can we please get this rolled out. This is important and urgent. I don't know how to mark this a showstopper. |
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.
Thanks for the submission @sandipagarwal. I have added a couple of comments on your code.
Sorry for the late response to this, I have been traveling. I'll get this merged and released as soon as the comments are addressed.
@@ -13,7 +13,7 @@ LICENSE: | |||
""" | |||
|
|||
import argparse | |||
import os | |||
# import os |
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.
Remove instead of commenting out.
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.
Done.
if os.path.exists('.git/index.lock'): | ||
os.remove('.git/index.lock') | ||
# if os.path.exists('.git/index.lock'): | ||
# os.remove('.git/index.lock') |
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.
Remove instead of commenting out.
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.
Done.
@@ -72,8 +72,8 @@ def main(): | |||
sys.exit(0) | |||
|
|||
# Remove the index.lock when it's exists |
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.
This comment should also be removed.
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.
Done.
|
||
# Stash any unstaged changes while we look at the tree | ||
with _stash_unstaged(): | ||
# Find Python files | ||
for filename in _get_list_of_committed_files(): | ||
try: | ||
if _is_python_file(filename) and \ | ||
not _is_ignored(filename, ignored_files): | ||
if _is_python_file(filename) and not _is_ignored(filename, ignored_files): |
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.
Why is this done? It seems like this line exceeds 80 chars. Please revert.
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.
My bad. Editor was configured so.
Done.
Thanks @sebdah. I have fixed all review comments from you. |
This has now been released in version 2.2.1. Thanks again @sandipagarwal for the submission! |
Thanks @sebdah. :) |
The following have been fixed.