functools test coverage

[Thorney]

BPO	12428
Nosy	@rhettinger, @ncoghlan, @pitrou, @jackdied, @ezio-melotti, @merwok, @ericsnowcurrently
Files	functools.diff: Diff against ddb8a29a6bc5 functools2.diff: Diff against 2428c239d848 12428.patch: Diff against bd353f12c007 issue12428.patch: Diff against 78263:00db71b3c5bd functools.patch: Diff against 5284e65e865b

^{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']

[Thorney]

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

[ncoghlan]

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]

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.

[Thorney]

Cheers for the comments Eric. I've modified the patch accordingly.

[pitrou]

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]

Ezio and I made further minor comments that can be handled by the person doing the commit; I’d like to do it.

[ncoghlan]

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...

[ncoghlan]

Also, the closure based implementation should be decorated with @staticmethod (see http://bugs.python.org/issue11704) and the tests updated accordingly.

[Thorney]

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]

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

[Thorney]

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]

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.

[Thorney]

Back to a simpler closure implementation of partial and skip the repr test for python implementation.

[python-dev]

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]

Sorry for the delay. I have now committed the patch to 3.4 (default). Thank you!

[ericsnowcurrently]

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?

[Thorney]

• 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?

[ericsnowcurrently]

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.

[pitrou]

> 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.