Skip to content

bpo-33365: print the header values besides the keys #6611

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

Merged

Conversation

lethliel
Copy link
Contributor

@lethliel lethliel commented Apr 26, 2018

With debuglevel=1 only the header keys got printed. With this change the header values get printed as well and the single header entries get '\n' as a separator.

https://bugs.python.org/issue33365

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

@lethliel
Copy link
Contributor Author

I have signed the CLA already.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a news entry.

@@ -321,7 +321,7 @@ def begin(self):

if self.debuglevel > 0:
for hdr in self.headers:
print("header:", hdr, end=" ")
print("header:", hdr + ":", self.headers.get(hdr), end="\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end="\n" is needless.

@lethliel lethliel force-pushed the fix_header_print_in_http_client branch from 3fe31ad to a128002 Compare May 8, 2018 13:18
@lethliel
Copy link
Contributor Author

@serhiy-storchaka: I added the news entry and removed the end="\n" in the print statement

@@ -0,0 +1 @@
print the header values besides the keys (debuglevel=1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks unclear out of the context. Please explain what is changed.

Add a period, start the sentence with a title case, and please add "Path by yourname."

@lethliel lethliel force-pushed the fix_header_print_in_http_client branch from a128002 to e638029 Compare May 23, 2018 09:14
@lethliel
Copy link
Contributor Author

@serhiy-storchaka: I improved the news entry. Can you please have a look again?

@serhiy-storchaka
Copy link
Member

Would be nice to add also a test. Something like the existing test test_tunnel_debuglog in Lib/test/test_httplib.py. It test debuglevel with tunneling, but you should now get the similar output without tunneling. Headers are printed in three places in the code, and it would be nice to add tests for all three cases.

Seems that the same header is printed twice in HTTPResponse.begin() (first time the raw header line, second time the parsed header), this can lead to confusion.

@lethliel lethliel force-pushed the fix_header_print_in_http_client branch from 6cf88d5 to 743769e Compare June 19, 2018 09:34
@lethliel
Copy link
Contributor Author

@serhiy-storchaka Sorry for the long delay, but I was busy the last weeks. I added a test and adopted your updated new entry. Thanks.

Where does the raw header line get printed in HTTPResponse.begin()?

@serhiy-storchaka
Copy link
Member

Seems I just missed that other prints print other lines.

@@ -344,6 +344,17 @@ def test_invalid_headers(self):
with self.assertRaisesRegex(ValueError, 'Invalid header'):
conn.putheader(name, value)

def test_headers_debuglevel(self):
body = b'HTTP/1.1 200 OK\r\nFirst: val\r\n: nval\r\nSecond: val\r\n\r\n'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What : nval means?

I think it would be better to write this literal as multiple lines, splitting at newlines.

body = (b'HTTP/1.1 200 OK\r\n'
        b'First: val\r\n'
        ...

with debuglevel=1 only the header keys got printed. With
this change the header values get printed as well and the single
header entries get '\n' as a separator.
@lethliel lethliel force-pushed the fix_header_print_in_http_client branch from 743769e to 1456e95 Compare June 19, 2018 12:17
@lethliel
Copy link
Contributor Author

@serhiy-storchaka: fixed the copy and paste error in body (removed the : nval) and splited it to multiple lines

@serhiy-storchaka serhiy-storchaka merged commit 936f03e into python:master Jun 19, 2018
@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Jun 19, 2018
@miss-islington
Copy link
Contributor

Thanks @lethliel for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @lethliel for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 19, 2018
with debuglevel=1 only the header keys got printed. With
this change the header values get printed as well and the single
header entries get '\n' as a separator.
(cherry picked from commit 936f03e)

Co-authored-by: Marco Strigl <[email protected]>
@bedevere-bot
Copy link

GH-7792 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 19, 2018
with debuglevel=1 only the header keys got printed. With
this change the header values get printed as well and the single
header entries get '\n' as a separator.
(cherry picked from commit 936f03e)

Co-authored-by: Marco Strigl <[email protected]>
@bedevere-bot
Copy link

GH-7793 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit that referenced this pull request Jun 19, 2018
with debuglevel=1 only the header keys got printed. With
this change the header values get printed as well and the single
header entries get '\n' as a separator.
(cherry picked from commit 936f03e)

Co-authored-by: Marco Strigl <[email protected]>
miss-islington added a commit that referenced this pull request Jun 19, 2018
with debuglevel=1 only the header keys got printed. With
this change the header values get printed as well and the single
header entries get '\n' as a separator.
(cherry picked from commit 936f03e)

Co-authored-by: Marco Strigl <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants