-
Notifications
You must be signed in to change notification settings - Fork 372
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
Minor bug fixes #536
Minor bug fixes #536
Conversation
secure = False | ||
if secure_warning: | ||
logger.warn("httplib is not built with ssl support") | ||
secure_warning = False |
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.
@hglkrijger I can see you set it False
here, but where does it ever achieve 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.
line 32
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.
@hglkrijger Hmm, how did I miss that? :P
@@ -107,7 +107,8 @@ def parse_ext_status(ext_status, data): | |||
if substatus_list is None: | |||
return | |||
for substatus in substatus_list: | |||
ext_status.substatusList.append(parse_ext_substatus(substatus)) | |||
if substatus is not None: |
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.
@hglkrijger Would a better check be to see if ext_status.substatuslist
is a list (that is, check for vs. check absence)?
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.
The issue we see is that substatus
is None
, and the parse in line 111 fails.
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.
@hglkrijger I understand. But 111 needs a list...so why not check (if possible) that ext_status.substatusList
is a list (vs. simply ensuring it's not None
)?
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.
@hglkrijger Nevermind. Senility.
/cc @jinhyunr @brendandixon