Repository navigation
functools test coverage #56637
Description
| BPO | 12428 |
|---|---|
| Nosy | @rhettinger, @ncoghlan, @pitrou, @jackdied, @ezio-melotti, @merwok, @ericsnowcurrently |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2012-11-13.20:37:48.064>
created_at = <Date 2011-06-28.10:07:17.610>
labels = ['type-bug', 'tests']
title = 'functools test coverage'
updated_at = <Date 2012-11-14.07:16:36.192>
user = 'https://bugs.python.org/Thorney'bugs.python.org fields:
activity = <Date 2012-11-14.07:16:36.192>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2012-11-13.20:37:48.064>
closer = 'pitrou'
components = ['Tests']
creation = <Date 2011-06-28.10:07:17.610>
creator = 'Thorney'
dependencies = []
files = ['22505', '22801', '25185', '26495', '26697']
hgrepos = []
issue_num = 12428
keywords = ['patch']
message_count = 19.0
messages = ['139353', '139358', '139359', '141425', '149082', '149105', '153778', '153779', '158101', '166254', '166264', '166318', '167473', '175523', '175524', '175529', '175530', '175532', '175543']
nosy_count = 9.0
nosy_names = ['rhettinger', 'ncoghlan', 'pitrou', 'jackdied', 'ezio.melotti', 'eric.araujo', 'Thorney', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12428'
versions = ['Python 3.4']
Activity
Thorney commentedon Jun 28, 2011
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:
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.
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:
Added two new tests to TestLRU.
Add testing of Python implementation of cmp_to_key. I copied how test_warnings.py tests C and Python implementations of the same function.
Made the importing of functools itself far less clear
ncoghlan commentedon Jun 28, 2011
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.
rhettinger commentedon Jun 28, 2011
We don't care. The docs make very few
guarantees beyond the core functionality.
Everything else is an implementation detail.
Thorney commentedon Jul 30, 2011
Cheers for the comments Eric. I've modified the patch accordingly.
pitrou commentedon Dec 9, 2011
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.
merwok commentedon Dec 9, 2011
Ezio and I made further minor comments that can be handled by the person doing the commit; I’d like to do it.
ncoghlan commentedon Feb 20, 2012
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:
ncoghlan commentedon Feb 20, 2012
Also, the closure based implementation should be decorated with @staticmethod (see http://bugs.python.org/issue11704) and the tests updated accordingly.
Thorney commentedon Apr 12, 2012
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 bpo-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.
pitrou commentedon Jul 23, 2012
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
The module is named _thread in 3.x, so this shouldn't have been changed (but admittedly it's thread in 2.x).
Thorney commentedon Jul 24, 2012
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?
pitrou commentedon Jul 24, 2012
Le mardi 24 juillet 2012 à 06:00 +0000, Brian Thorne a écrit :
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.
Why not, if there's an easy way to do so.
Thorney commentedon Aug 5, 2012
Back to a simpler closure implementation of partial and skip the repr test for python implementation.
python-dev commentedon Nov 13, 2012
New changeset fcfaca024160 by Antoine Pitrou in branch 'default':
Issue bpo-12428: Add a pure Python implementation of functools.partial().
http://hg.python.org/cpython/rev/fcfaca024160
pitrou commentedon Nov 13, 2012
Sorry for the delay. I have now committed the patch to 3.4 (default). Thank you!
ericsnowcurrently commentedon Nov 13, 2012
The following from the changeset left me with questions:
Thorney commentedon Nov 14, 2012
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.
What are the main considerations to properly answer these last questions? Performance comparison between the implementations, maintainability?
ericsnowcurrently commentedon Nov 14, 2012
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?
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.
pitrou commentedon Nov 14, 2012
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.