-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
feat(git): Omit git info when git isnt installed #187
Conversation
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 contribution! If you could avoid the exc_info
change, I'd love to accept the rest. Also -- looks like you branched off of master between when I broke it and when I fixed it. Would you mind rebasing off HEAD
?
coveralls/git.py
Outdated
@@ -94,7 +94,8 @@ def git_info(): | |||
}] | |||
if not all(head.values()): | |||
log.warning('Failed collecting git data. Are you running ' | |||
'coveralls inside a git repository?', exc_info=ex) | |||
'coveralls inside a git repository? Is git installed?', | |||
exc_info=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.
Huh, TIL you can pass in True
instead of the exception instance. Looks like this causes an extra call to sys.exc_info
though, which is needless when we could just use the already-captured ex
from above. Could you revert that part of your change?
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.
I find it cleaner this way, but sure. Will push a commit in a few minutes!
Addresses this comment: TheKevJames#174 (comment) Previously, git info was omitted if git was installed but the .git directory was missing. This fix will also omit git info in case git itself is missing (unless, of course, the environment args are there).
@TheKevJames rebased and reverted the |
@nirizr nope, looks great. Thanks again for the fix! |
(@nirizr oh, and a heads up that this will be released with 1.5.1, which I'm just pushing now) |
Addresses this comment: #174 (comment)
Previously, git info was omitted if git was installed but the .git directory was missing.
This fix will also omit git info in case git itself is missing (unless, of course, the environment args are there).