[issue12428] functools test coverage

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Eric Snow]

Eric Snow added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Brian Thorne added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Eric Snow]

Eric Snow added the comment:

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?

--
nosy: +eric.snow

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Antoine Pitrou]

Antoine Pitrou added the comment:

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

--
resolution:  -> fixed
stage: commit review -> committed/rejected
status: open -> closed
versions: +Python 3.4 -Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Brian Thorne added the comment:

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

--
Added file: http://bugs.python.org/file26697/functools.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Antoine Pitrou]

Antoine Pitrou  added the comment:

Le mardi 24 juillet 2012 à 06:00 +, 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Brian Thorne  added the comment:

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?

--
Added file: http://bugs.python.org/file26495/issue12428.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Changes by Brian Thorne :


--
nosy: +jackdied

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Ezio Melotti]

Changes by Ezio Melotti :


--
nosy: +ezio.melotti

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Brian Thorne  added the comment:

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.

--
Added file: http://bugs.python.org/file25185/12428.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Nick Coghlan]

Nick Coghlan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Nick Coghlan]

Nick Coghlan  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Éric Araujo]

Éric Araujo  added the comment:

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

--
nosy: +eric.araujo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Antoine Pitrou]

Antoine Pitrou  added the comment:

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.

--
nosy: +pitrou
stage:  -> commit review
versions: +Python 3.3 -Python 3.2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

Brian Thorne  added the comment:

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

--
Added file: http://bugs.python.org/file22801/functools2.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Raymond Hettinger]

Raymond Hettinger  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Nick Coghlan]

Nick Coghlan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue12428] functools test coverage

[Brian Thorne]

New submission from Brian Thorne :

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

--
components: Tests
files: functools.diff
keywords: patch
messages: 139353
nosy: Thorney, ncoghlan, rhettinger
priority: normal
severity: normal
status: open
title: functools test coverage
type: behavior
versions: Python 3.2
Added file: http://bugs.python.org/file22505/functools.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com