Skip to content

bpo-33897: Add a 'force' keyword argument to logging.basicConfig().#7873

Merged
vsajip merged 1 commit intopython:masterfrom
corona10:bpo-33897
Jun 25, 2018
Merged

bpo-33897: Add a 'force' keyword argument to logging.basicConfig().#7873
vsajip merged 1 commit intopython:masterfrom
corona10:bpo-33897

Conversation

@corona10
Copy link
Copy Markdown
Member

@corona10 corona10 commented Jun 23, 2018 •
edited by bedevere-bot
Loading

@corona10 corona10 force-pushed the bpo-33897 branch 2 times, most recently from fad9c50 to a55dfa1 Compare June 23, 2018 10:22
vsajip
vsajip requested changes Jun 23, 2018
View reviewed changes
Comment thread Doc/library/logging.rst Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

... has handlers configured, unless the keyword argument force is set to True.

Comment thread Doc/library/logging.rst Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

... is specified as true, any existing handlers attached to the root logger are removed and closed, before carrying out the configuration as specified by the other arguments.

Comment thread Lib/logging/__init__.py Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

... has handlers configured, unless the keyword argument force is set to True.

Comment thread Lib/logging/__init__.py Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

... is specified as true, any existing handlers attached to the root logger are removed and closed, before carrying out the configuration as specified by the other arguments.

Comment thread Lib/test/test_logging.py Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

Is this seek() call needed? Doesn't getvalue() get the entire output, anyway?

Comment thread Misc/NEWS.d/next/Library/2018-06-23-18-09-28.bpo-33897.Hu0yvt.rst Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

Added a 'force' keyword argument to logging.basicConfig().

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 23, 2018

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10 corona10 changed the title bpo-33897: Add a 'force' keyword argument for basicConfig(). bpo-33897: Added a 'force' keyword argument to logging.basicConfig(). Jun 23, 2018
pablogsal
pablogsal reviewed Jun 23, 2018
View reviewed changes
Comment thread Lib/test/test_logging.py Outdated
Copy link
Copy Markdown
Member

@pablogsal pablogsal Jun 23, 2018

Choose a reason for hiding this comment

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

Suggestion: Can we log on debug on this line? This way is very clear that the INFO:root:info comes after calling logging.basicConfig(level=logging.INFO, stream=sys.stdout, force=True) and not before.

Copy link
Copy Markdown
Member

@vsajip vsajip Jun 23, 2018

Choose a reason for hiding this comment

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

Good point. Perhaps better yet, we could assert that output.getvalue() contains only the WARNING line after this call, or immediately before the logging.info() call which follows this one.

@corona10 corona10 force-pushed the bpo-33897 branch 2 times, most recently from e6891de to 5e40306 Compare June 24, 2018 09:43
@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jun 24, 2018

@vsajip @pablogsal
I updated the unit test to detect the change of handler by using StringIO.

vsajip
vsajip reviewed Jun 24, 2018
View reviewed changes
Comment thread Lib/test/test_logging.py Outdated
Copy link
Copy Markdown
Member

@vsajip vsajip Jun 24, 2018

Choose a reason for hiding this comment

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

Please use WARNING here, WARN is only around for backward compatibility.

@corona10 corona10 changed the title bpo-33897: Added a 'force' keyword argument to logging.basicConfig(). bpo-33897: Add a 'force' keyword argument to logging.basicConfig(). Jun 24, 2018
@corona10
Copy link
Copy Markdown
Member Author

corona10 commented Jun 24, 2018

@vsajip Thanks updated!

@vsajip vsajip merged commit cf67d6a into python:master Jun 25, 2018
@corona10 corona10 deleted the bpo-33897 branch June 25, 2018 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vsajip vsajip vsajip requested changes

@pablogsal pablogsal pablogsal left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@corona10 @bedevere-bot @vsajip @pablogsal @the-knights-who-say-ni