[issue24255] Replace debuglevel-related logic with logging

[Aldwin Pollefeyt]

Aldwin Pollefeyt  added the comment:

PR waiting review, Stage should be 'patch review'

--
nosy: +aldwinaldwin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Sanyam Khurana  added the comment:

Yeah, that is understandable. I've reverted major chunk of it. Just in the http 
client file, I've added blank lines where ever necessary (which I think won't 
change the blame information for any of the code :))

Would that be okay?

Also, I did a change in the test to redirect the `output` from `test.support` 
to `stdout` in order to test the changes, but seems like the CI failed because 
the environment of test was changed. Is there a better method to accomplish it?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[R. David Murray]

R. David Murray  added the comment:

We generally do not fix "linting errors" unless they reveal logic errors or we 
touch the lines of code for some other reason.  We also follow the existing 
style of a module rather than any particular style guide (the stdlib modules 
are often older than all of the style guides...even PEP8).  In short, omit 
those changes from your PR, and don't bother creating a separate one, it would 
just get rejected ;)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Sanyam Khurana  added the comment:

Hey Conrad,

I've merged your fixes as well. They are in the PR now. Also added your name in 
the NEWS entry :)

@Vinay, I noticed that there are linting errors in `test_logging.py`. Does it 
make sense to issue a separate Pull Request to fix those? Currently, I've fixed 
them in the current Pull Request. Please let me know :)

--
nosy: +vinay.sajip

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Conrad Ho]

Conrad Ho  added the comment:

Hi Sanyam, were you able to fix the CI errors?

The fixes for the infinite loop that you are seeing in your PR CI run and the 
changes to test correct logging (vs testing stdout) etc are in my original 
patch already. I've checked that the test suite passes with my patch.

Otherwise, should I try to submit a PR / what's the best way to move this 
forward? It's my first contribution to cpython!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Sanyam Khurana  added the comment:

Sure Conrad,

Yeah, indeed. The patch didn't apply cleanly, so I"ve done it manually.

I've raised the PR here: https://github.com/python/cpython/pull/8633

I'll check your patch and merge :)

Thanks for your help too!

I'm adding Python 3.7 and Python 3.8 for this patch.

--
stage: patch review -> needs patch
versions: +Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Change by Sanyam Khurana :


--
pull_requests: +8138
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Conrad Ho]

Conrad Ho  added the comment:

@Eryn in the news blurb thing I'm going to say
"original patch done by Eryn Wells." Your employer should be okay with that 
right? :D

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Conrad Ho]

Conrad Ho  added the comment:

Thanks Eryn!

@Sanyam if you apply the original patch directly that will currently result in 
some merge failures, and there are test fixes etc that I did on the second 
patch. Think we should combine them.

I just made the chgs suggested by David on my own forked repo. Do you want me 
to submit a PR directly?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Sanyam Khurana  added the comment:

That's okay Eryn. We really appreciate all your help.

I will take this patch forward :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Eryn Wells]

Eryn Wells  added the comment:

Actually, I spoke too soon. My current employer isn't too keen on us 
contributing to open source, so I can't do this. Sorry!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Eryn Wells]

Eryn Wells  added the comment:

Hi, it sounds like my original patch is the preferred approach. I can put up a 
GitHub PR for that.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[R. David Murray]

R. David Murray  added the comment:

Conrad: thanks for the effort, but using f-strings with logging is 
counterproductive.  The idea behind logging is that the logged strings are not 
materialized unless something actually wants to output them.  Using fstrings 
means you are doing all the work of formatting the string regardless of whether 
or not the string is actually going to get written anywhere.  The original 
patch also retains the debug guards that minimize overhead when debugging is 
not turned on, which it doesn't look like your patch does.

Regardless, what we need at this stage is a github PR, not a patch :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Conrad Ho]

Conrad Ho  added the comment:

Hi,

I have referenced the original patch and created an updated patch that uses the 
logging module + f-strings in place of the print statements in the http.client 
module. Also updated the relevant tests for print/logging in test_httplib to 
reflect these changes.

The HTTPHandlerTest testcase from test_logging was also affected. In the 
testcase, it gets a logger with name 'http' and adds a logging.HTTPHandler to 
it. In our patch, we create a http.client logger, which happens to be 
considered a child of this testcase logger under the logger naming hierarchy.

This causes http.client logging events to propagate up to the 'http' logger and 
for the testcase to loop infinitely (ie. the HTTPHandler calls http.client 
functions internally. These functions log events using the http.client logger, 
which propagate up to the testcase http logger which calls HttpHandler again).

I have simply changed the testcase getLogger name to not be 'http' and clash 
with that http.client module logger.

--
nosy: +Conrad Ho
Added file: https://bugs.python.org/file47727/http-client-logging-v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Megan Sosey]

Megan Sosey  added the comment:

I'm going to work on this one since the original reporter last commented 3 
years ago. I took a quick look at how the other modules are handling logging to 
see if I could make it consistent, and they all do it a bit differently. It 
might be worthwhile considering normalizing logging across the modules at some 
point. I'm working with the python 3.8 currently on master.

--
nosy: +msosey

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Sanyam Khurana]

Sanyam Khurana  added the comment:

Hey Erin,

Can you please convert your patch to a PR on Github?

--
nosy: +CuriousLearner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Xavier G. Domingo]

Change by Xavier G. Domingo :


--
nosy: +xgdomingo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Simon Lipp]

Change by Simon Lipp :


--
nosy: +sloonz

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Eryn Wells]

Eryn Wells added the comment:

Here's a patch that replaces all the debuglevel stuff with logging. There's a 
single module-level logger that handles it all.

I wasn't sure if it would be okay to break API compatibility, so I kept the 
debuglevel flag and the set_debuglevel() method even though they don't do 
anything.

Most of the information that was being print()ed is still there, but I cleaned 
it up a little and added a few more messages.

The INFO level messages are pretty sparse, with DEBUG providing a lot more raw 
data from the request.

--
keywords: +patch
Added file: http://bugs.python.org/file39651/http-client-logging.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Berker Peksag]

Changes by Berker Peksag :


--
nosy: +berker.peksag

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Eryn Wells]

Eryn Wells added the comment:

Hi. This is my first issue, but I'd be willing to have a go at updating this 
module. Do either of you have specific ideas about what changes you want here? 

My thought was to import logging, create a logger object, and start by 
replacing all the self.debuglevel stuff with appropriate log statements. Does 
that make sense? Do each of the classes need separate loggers, or is just one 
module-level logger enough?

--
nosy: +erynofwales

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Demian Brecht]

Changes by Demian Brecht :


--
keywords: +easy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[R. David Murray]

R. David Murray added the comment:

+1

--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24255] Replace debuglevel-related logic with logging

[Demian Brecht]

New submission from Demian Brecht:

Far too many times have I wished that changing the logging output in 
http.client was controllable through logging configuration rather than code 
changes modifying a connection's debuglevel.

It would be nice if the http package was brought up to date and had 
debuglevel-related code replaced with logging and sane logging was added 
throughout.

--
components: Library (Lib)
messages: 243734
nosy: demian.brecht
priority: normal
severity: normal
stage: needs patch
status: open
title: Replace debuglevel-related logic with logging
type: enhancement
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com