-
Notifications
You must be signed in to change notification settings - Fork 259
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
Improve error handling #13
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.
That looks pretty good, thanks for working on this. Please address my feedback and I will happily merge this :)
@@ -29,7 +30,7 @@ def printf(fmt, *args, file=sys.stdout): | |||
|
|||
def is_html(response): | |||
''' Return True if the response is a HTML webpage ''' | |||
return '<html>' in response.text | |||
return "html" in response.headers["Content-Type"] |
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.
Should it be text/html
?
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're right, good catch.
result = self.do_task(task, *self.args) | ||
try: | ||
result = self.do_task(task, *self.args) | ||
except Exception as e: |
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.
We don't need e
here, we could just except:
?
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.
Oh, you're right, good catch
verification = verify_response(response) | ||
if not verification[0]: | ||
printf(verification[1], url, filepath) | ||
return [] |
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.
Here and below: Instead of using verification[0] and verification[1] we could use unpacking:
valid, error_message = verify_response(response)
if not valid:
printf(error_message, url, filepath)
return []
elif not response.text.startswith('ref:'): | ||
printf('error: %s/.git/HEAD is not a git HEAD file\n', url, file=sys.stderr) |
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.
Could we keep this check?
return 1 | ||
|
||
|
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.
nitpick: remove the empty newline
Could you please apply my suggested changes and update the pull request? |
I would love if this could be approved, this is an amazing change |
Since the author wouldn't apply my suggested changed, I did that myself. Thanks for the contribution! |
I added an exception handler to Worker.run so a single failed task doesn't crash the whole worker, and also added logic to check for HTML responses (error pages that give a 200 response code instead of 404) and zero-length responses (I've run into them, surprisingly).