bpo-38698: Prevent UnboundLocalError to pop up in parse_message_id #17277
Conversation
|
I'm not sure if this is NEWS worthy and can't apply "skip news" by myself. |
| @@ -2113,7 +2113,8 @@ def parse_message_id(value): | |||
| except errors.HeaderParseError: | |||
| message_id.defects.append(errors.InvalidHeaderDefect( | |||
| "Expected msg-id but found {!r}".format(value))) | |||
| message_id.append(token) | |||
| else: | |||
There was a problem hiding this comment.
I'd like you to change the code like below.
message_id = MessageID()
try:
token, value = get_msg_id(value)
+ message_id.append(token)
except errors.HeaderParseError:
message_id.defects.append(errors.InvalidHeaderDefect(
"Expected msg-id but found {!r}".format(value)))
- else:
- message_id.append(token)
return message_id
There was a problem hiding this comment.
What is wrong with having the append call inside the else branch? Especially since the body of the try statement contains just the instruction that might fail. This seems to me like an extra nitpicky comment for a personal flavour, not necessarily an issue with the code itself.
There was a problem hiding this comment.
It didn't mean your code was wrong. I apologize if you feel bad.
As I read the code again, the original author's intention seems to be the code you proposed.
| @@ -2113,7 +2113,8 @@ def parse_message_id(value): | |||
| except errors.HeaderParseError: | |||
| message_id.defects.append(errors.InvalidHeaderDefect( | |||
| "Expected msg-id but found {!r}".format(value))) | |||
| message_id.append(token) | |||
| else: | |||
There was a problem hiding this comment.
It didn't mean your code was wrong. I apologize if you feel bad.
As I read the code again, the original author's intention seems to be the code you proposed.
|
@corona10 We do need a NEWS item for this, it is a bugfix. Skip News label should be used only for trivial doc changes. The changes look good, thanks for your contribution @PCManticore, can you please add a NEWS entry? Devguide has explanation on using the |
parse_message_id() was improperly using a token defined inside an exception handler, which was raising `UnboundLocalError` on parsing an invalid value.
PCManticore
force-pushed
the
bpo-38698-unbound-local-error
branch
from
14ad49b
to
1dddda0
Compare
|
@maxking Makes sense, all done! |
maxking
added
|
Sorry, I can't merge this PR. Reason: |
maxking
removed
the
|
I think this can be backported to 3.8 too which has the same issue. |
|
You are of course right @tirkarthi :) |
|
Thanks @PCManticore for the PR |
|
GH-17476 is a backport of this pull request to the 3.8 branch. |
…ythonGH-17277) parse_message_id() was improperly using a token defined inside an exception handler, which was raising `UnboundLocalError` on parsing an invalid value. https://bugs.python.org/issue38698 (cherry picked from commit bb81549) Co-authored-by: Claudiu Popa <pcmanticore@gmail.com>
…H-17277) parse_message_id() was improperly using a token defined inside an exception handler, which was raising `UnboundLocalError` on parsing an invalid value. https://bugs.python.org/issue38698 (cherry picked from commit bb81549) Co-authored-by: Claudiu Popa <pcmanticore@gmail.com>
…ythonGH-17277) parse_message_id() was improperly using a token defined inside an exception handler, which was raising `UnboundLocalError` on parsing an invalid value. https://bugs.python.org/issue38698
parse_message_id() was improperly using a token defined inside an exception
handler, which was raising
UnboundLocalErroron parsing an invalid value.https://bugs.python.org/issue38698
Automerge-Triggered-By: @maxking