Skip to content
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

bpo-30520: Implemented pickling for loggers. #1956

Merged
merged 13 commits into from Jun 6, 2017

Conversation

Projects
None yet
8 participants
@vsajip
Copy link
Member

commented Jun 5, 2017

No description provided.

@vsajip vsajip self-assigned this Jun 5, 2017

@vsajip vsajip requested a review from pitrou Jun 5, 2017

@@ -323,6 +323,8 @@ is the module's name in the Python package namespace.

.. versionadded:: 3.2

.. versionadded:: 3.7

This comment has been minimized.

Copy link
@pitrou

pitrou Jun 5, 2017

Member

That should probably be a versionchanged.

This comment has been minimized.

Copy link
@vsajip

vsajip Jun 5, 2017

Author Member

Done.

@pitrou

pitrou approved these changes Jun 5, 2017

def test_pickling(self):
for name in ('', 'root', 'foo', 'foo.bar', 'baz.bar'):
logger = logging.getLogger(name)
s = pickle.dumps(logger)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

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, ('',)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

Maybe without arguments at all?

return getLogger, ()
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:

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

This implicitly creates the global logger with name self.name. Is it okay?

This comment has been minimized.

Copy link
@vsajip

vsajip Jun 5, 2017

Author Member

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')

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

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):

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

__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().

This comment has been minimized.

Copy link
@vsajip

vsajip Jun 5, 2017

Author Member

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'):

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

Is self.logger pickleable? If not, it may make sense to add an explicit test for this.

This comment has been minimized.

Copy link
@vsajip

vsajip Jun 5, 2017

Author Member

No, it's not pickleable, as it's created using Logger.__init__(), which users are not supposed to use.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Jun 5, 2017

Member

It is created via Logger constructor, not via getLogger(). logging.Logger('blah') is not logging.getLogger('blah').

@serhiy-storchaka
Copy link
Member

left a comment

LGTM.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member

commented Jun 5, 2017

Ah, just please add a Misc/NEWS entry.

zooba and others added some commits Jun 5, 2017

bpo-30557: faulthandler now correctly filters and displays exception …
…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.
bpo-30417: Disable 'cpu' and 'tzdata' resources on Travis (GH-1928)
Also weakens the 'should this be run?' regex to allow all builds when .travis.yml changes.
bpo-30095: Make CSS classes used by calendar.HTMLCalendar customizable (
GH-1439)

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 (#1855)
* Simplify X.509 extension handling code

The previous implementation had grown organically over time, as OpenSSL's API evolved.

* Delete even more code

@vsajip vsajip merged commit 6260d9f into python:master Jun 6, 2017

2 checks passed

bedevere/issue-number Issue number 30520 found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vsajip vsajip deleted the vsajip:pickle-logger branch Jun 6, 2017

mlouielu added a commit to mlouielu/cpython that referenced this pull request Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.