bpo-30520: Implemented pickling for loggers. by vsajip · Pull Request #1956 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits13 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| @@ -323,6 +323,8 @@ is the module's name in the Python package namespace. |
|---|
| .. versionadded:: 3.2 |
| .. versionadded:: 3.7 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should probably be a versionchanged.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| def test_pickling(self): |
|---|
| for name in ('', 'root', 'foo', 'foo.bar', 'baz.bar'): |
| logger = logging.getLogger(name) |
| s = pickle.dumps(logger) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please test it for all protocols:
for proto in range(pickle.HIGHEST_PROTOCOL + 1):
s = pickle.dumps(logger, proto)
| @@ -1583,6 +1590,9 @@ def __init__(self, level): |
|---|
| """ |
| Logger.__init__(self, "root", level) |
| def __reduce__(self): |
| return getLogger, ('',) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe without arguments at all?
| def __reduce__(self): |
|---|
| # In general, only the root logger will not be accessible via its name. |
| # However, the root logger's class has its own __reduce__ method. |
| if getLogger(self.name) is not self: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implicitly creates the global logger with name self.name. Is it okay?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a problem with it.
| # In general, only the root logger will not be accessible via its name. |
|---|
| # However, the root logger's class has its own __reduce__ method. |
| if getLogger(self.name) is not self: |
| raise pickle.PicklingError('logger cannot be pickled') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pickle module is used only here. It isn't used if pickling is not involved. May be make an exception from PEP 8 and import it locally?
| @@ -4086,6 +4086,13 @@ def test_invalid_names(self): |
|---|
| self.assertRaises(TypeError, logging.getLogger, any) |
| self.assertRaises(TypeError, logging.getLogger, b'foo') |
| def test_pickling(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__reduce__ is used also in the copy module. Maybe add tests for copy.copy() and copy.deepcopy()? Even unpickleable loggers can be copied if add special __copy__ and __deepcopy__ methods or register the classes with copyreg.pickle().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loggers are singletons, so copy operations shouldn't actually make copies, anyway. Seems like a separate issue (or at least use case).
| @@ -4086,6 +4086,13 @@ def test_invalid_names(self): |
|---|
| self.assertRaises(TypeError, logging.getLogger, any) |
| self.assertRaises(TypeError, logging.getLogger, b'foo') |
| def test_pickling(self): |
| for name in ('', 'root', 'foo', 'foo.bar', 'baz.bar'): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is self.logger pickleable? If not, it may make sense to add an explicit test for this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not pickleable, as it's created using Logger.__init__(), which users are not supposed to use.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is created via Logger constructor, not via getLogger(). logging.Logger('blah') is not logging.getLogger('blah').
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Ah, just please add a Misc/NEWS entry.
zooba and others added 10 commits
…codes on Windows (#1924)
bpo-30557: faulthandler now correctly filters and displays exception codes on Windows
Adds test for non-fatal exceptions.
Adds bpo number to comment.
Also weakens the 'should this be run?' regex to allow all builds when .travis.yml changes.
Several class attributes have been added to calendar.HTMLCalendar that allow customization of the CSS classes used in the resulting HTML. This can be done by subclasses HTMLCalendar and overwriting those class attributes (Patch by Oz Tiram).
- Simplify X.509 extension handling code
The previous implementation had grown organically over time, as OpenSSL's API evolved.
- Delete even more code
