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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:19 | admin | set | github: 56637 |
2012-11-14 07:16:36 | pitrou | set | messages:
+ msg175543 |
2012-11-14 03:51:05 | eric.snow | set | messages:
+ msg175532 |
2012-11-14 00:14:29 | Thorney | set | messages:
+ msg175530 |
2012-11-13 23:48:19 | eric.snow | set | nosy:
+ eric.snow messages:
+ msg175529
|
2012-11-13 20:37:48 | pitrou | set | status: open -> closed versions:
+ Python 3.4, - Python 3.3 messages:
+ msg175524
resolution: fixed stage: commit review -> resolved |
2012-11-13 20:37:09 | python-dev | set | nosy:
+ python-dev messages:
+ msg175523
|
2012-08-05 07:45:12 | Thorney | set | files:
+ functools.patch
messages:
+ msg167473 |
2012-07-24 17:33:29 | pitrou | set | messages:
+ msg166318 |
2012-07-24 06:00:25 | Thorney | set | files:
+ issue12428.patch
messages:
+ msg166264 |
2012-07-23 23:44:31 | pitrou | set | messages:
+ msg166254 |
2012-07-16 07:08:15 | Thorney | set | nosy:
+ jackdied
|
2012-04-14 04:15:16 | ezio.melotti | set | nosy:
+ ezio.melotti
|
2012-04-12 03:32:28 | Thorney | set | files:
+ 12428.patch
messages:
+ msg158101 |
2012-02-20 12:50:24 | ncoghlan | set | messages:
+ msg153779 |
2012-02-20 12:44:22 | ncoghlan | set | messages:
+ msg153778 |
2011-12-09 15:38:38 | eric.araujo | set | nosy:
+ eric.araujo messages:
+ msg149105
|
2011-12-09 09:53:35 | pitrou | set | versions:
+ Python 3.3, - Python 3.2 nosy:
+ pitrou
messages:
+ msg149082
stage: commit review |
2011-07-30 06:08:14 | Thorney | set | files:
+ functools2.diff
messages:
+ msg141425 |
2011-06-28 12:24:08 | rhettinger | set | messages:
+ msg139359 |
2011-06-28 12:16:13 | ncoghlan | set | messages:
+ msg139358 |
2011-06-28 10:07:17 | Thorney | create | |