| msg139353 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2011-06-28 10:07 |
| The test coverage for functools was down around ~60%, this is a patch to bring that up to ~98%. Made two changes to the Lib/functools.py file itself: 1) Moved the Python implementation of partial into Lib/functools.py from Lib/test/test_functools.py which gets imported over the same as the Python implementation of cmp_to_key. 2) In order to allow the blocking of _functools, I grouped and moved the import functions from of _functools to the end of the file. In the test_functools.py file: 3) Added two new tests to TestLRU. 4) Add testing of Python implementation of cmp_to_key. I copied how test_warnings.py tests C and Python implementations of the same function. 5) Made the importing of functools itself far less clear |
|
|
| msg139358 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
Date: 2011-06-28 12:16 |
| Raymond, do we care whether or not the pure Python version of functools.partial supports inheritance and instance testing? The constructor is technically documented as returning a "partial object" rather than a simple staticmethod instance with additional attributes. My own preference leans towards keeping the closure based implementation due to its simplicity, which is what makes it particularly useful as a cross-check on the C implementation. |
|
|
| msg139359 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2011-06-28 12:24 |
| > Raymond, do we care whether or not the > pure Python version of functools.partial > supports inheritance and instance testing? We don't care. The docs make very few guarantees beyond the core functionality. Everything else is an implementation detail. |
|
|
| msg141425 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2011-07-30 06:08 |
| Cheers for the comments Eric. I've modified the patch accordingly. |
|
|
| msg149082 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-12-09 09:53 |
| Brian's patch looks ok to me. There's a missing newline (or two) just before test_main() but that can easily be fixed on commit. |
|
|
| msg149105 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-12-09 15:38 |
| Ezio and I made further minor comments that can be handled by the person doing the commit; I’d like to do it. |
|
|
| msg153778 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2012-02-20 12:44 |
| Just noticed one minor nit with the patch: the pure Python version of functools.partial should support "func" as a keyword argument that is passed to the underlying object. The trick is to declare a positional only argument like this: def f(*args, **kwds): first, *args = args # etc... |
|
|
| msg153779 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2012-02-20 12:50 |
| Also, the closure based implementation should be decorated with @staticmethod (see http://bugs.python.org/issue11704) and the tests updated accordingly. |
|
|
| msg158101 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2012-04-12 03:32 |
| I've updated the patch to address the comments here and in the code review. I added more cross testing of the pure Python implementation of partial - as you pointed out inheritance wasn't supported so I changed from the simple closure to a class implementation. Instead of skipping repr tests for the pure Python implementation could we not just implement it? I did skip the pickle test for the Python implementation though. Nick, I wasn't sure how to decorate the partial object as a staticmethod so I don't think this patch addresses issue 11704. Also I didn't understand why Lock was being imported from _thread instead of thread. Since coverage went to 100% and the tests continue to all pass when I changed this. |
|
|
| msg166254 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-07-23 23:44 |
| Why does the pure Python version of partial have to be so complicated? I don't think the __class__, __setattr__ and __delattr__ are useful. As Raymond said, only the core, documented functionality needs to be preserved, not implementation details. Something else: - from _thread import allocate_lock as Lock + from thread import allocate_lock as Lock The module is named _thread in 3.x, so this shouldn't have been changed (but admittedly it's thread in 2.x). |
|
|
| msg166264 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2012-07-24 06:00 |
| Thanks Antoine, the __class__ attribute wasn't useful, I've removed that. Overriding the __setattr__ and __delattr__ gives consistent behaviour with the both versions - allowing the unittest reuse. Also I've changed thread back to _thread. Isn't more compatibility between the Python and C implementations desired? Is it an aim to document more of the functionality? An earlier version of this patch had a closure implementation of partial; I'm happy to revert to that if simplicity is preferred to compatibility? Should the caching decorators be tested from multiple threads? |
|
|
| msg166318 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-07-24 17:33 |
| Le mardi 24 juillet 2012 à 06:00 +0000, Brian Thorne a écrit : > Isn't more compatibility between the Python and C implementations > desired? IMHO, not when compatibility regards obscure details such as whether setting an attribute is allowed or not. I don't know why this was codified in the unit tests in the first place, perhaps Nick can shed some light. > Should the caching decorators be tested from multiple threads? Why not, if there's an easy way to do so. |
|
|
| msg167473 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2012-08-05 07:45 |
| Back to a simpler closure implementation of partial and skip the repr test for python implementation. |
|
|
| msg175523 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2012-11-13 20:37 |
| New changeset fcfaca024160 by Antoine Pitrou in branch 'default': Issue #12428: Add a pure Python implementation of functools.partial(). http://hg.python.org/cpython/rev/fcfaca024160 |
|
|
| msg175524 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-11-13 20:37 |
| Sorry for the delay. I have now committed the patch to 3.4 (default). Thank you! |
|
|
| msg175529 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2012-11-13 23:48 |
| The following from the changeset left me with questions: -from _functools import partial, reduce +try: + from _functools import reduce +except ImportError: + pass * Why the try block when there wasn't one before? * Should reduce be added to __all__ only conditionally? * Should the pure Python partial only be used if _functools.partial is not available? * Should _functools.partial be removed? |
|
|
| msg175530 - (view) |
Author: Brian Thorne (Thorney) |
Date: 2012-11-14 00:14 |
| > * Why the try block when there wasn't one before? > * Should reduce be added to __all__ only conditionally? My mistake, the try block should have just covered the import of partial - that is after all the exceptional circumstance we can deal with by using the pure python implementation. Possibly reduce could be handled in a similar way with a fallback python implementation? Otherwise your suggestion of conditionally adding it to __all__ makes sense to me. > * Should the pure Python partial only be used if _functools.partial is not available? > * Should _functools.partial be removed? What are the main considerations to properly answer these last questions? Performance comparison between the implementations, maintainability? |
|
|
| msg175532 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2012-11-14 03:51 |
| > Possibly reduce could be handled in a similar way with a fallback python > implementation? Otherwise your suggestion of conditionally adding it to __all__ > makes sense to me. In the meantime I'd expect the import of _functools.reduce to not be wrapped in a try block. Does that have an impact on coverage? >> * Should the pure Python partial only be used if _functools.partial is not available? >> * Should _functools.partial be removed? > > What are the main considerations to properly answer these last questions? Performance > comparison between the implementations, maintainability? Sorry, the first time through I missed the part of the patch that tries to import _functools.partial _after_ the pure Python version is defined. |
|
|
| msg175543 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-11-14 07:16 |
| > > Possibly reduce could be handled in a similar way with a fallback python > > implementation? Otherwise your suggestion of conditionally adding it to __all__ > > makes sense to me. > > In the meantime I'd expect the import of _functools.reduce to not be > wrapped in a try block. Does that have an impact on coverage? I tried to remove the try block, but when making the import unconditional the tests fail with an ImportError (because reduce doesn't have a pure Python implementation). I haven't investigated further, since the try block doesn't look like a real issue to me. |
|
|