Remove positional arg "func" from curry.__init__

[Digenis]

conflicted with kwargs['func']

example.py:

from toolz import curry
@curry
def foo(x, y, func=int, bar=str):
    return str(func(x*y))
foo(bar=float)(4.2, 3.8, func=round)
foo(func=int)(4.2, 3.8, bar=str)

The last line would throw TypeError: __init__() got multiple values for keyword argument 'func'
because curry.__init__(self, func, *args, **kwargs) names its first positional argument "func"
This effectively prevented creating a curry object with such a kwarg.
I didn't find other functions with the same problem.

> /home/digenis/src/toolz/toolz/example.py(1)<module>()
-> from toolz import curry
(pdb) c
Traceback (most recent call last):
 ...
  File "/home/digenis/src/toolz/toolz/functoolz.py", line 224, in __call__
    return curry(self._partial, *args, **kwargs)
TypeError: __init__() got multiple values for keyword argument 'func'
...
> /home/digenis/src/toolz/toolz/functoolz.py(224)__call__()
-> return curry(self._partial, *args, **kwargs)
(pdb) self._partial  # __init__ will receive this as the positional argument "func"
<functools.partial object at 0x7f16874ee0a8>
(pdb) pp args
()
(pdb) pp kwargs  # but it will also receive a kwarg named "func"
{'func': <type 'int'>}


Post mortem debugger finished. The example.py will be restarted
...

I abbreviated some line ranges with "..."

[Digenis]

Why did the wrong pypy builds succeed?

[mrocklin]

This looks good. The only thing I would suggest is to include your (very clear) example as a test to ensure that this behavior continues to work in the future.

[Digenis]

Do you mean a test for the signature of curry()?
To ensure it keeps this specific argspec
and maybe test some kwargs from #106. ok

[mrocklin]

I think that the example that you gave:

from toolz import curry
@curry
def foo(x, y, func=int, bar=str):
    return str(func(x*y))
foo(bar=float)(4.2, 3.8, func=round)
foo(func=int)(4.2, 3.8, bar=str)

Is a good test case here. I would just execute that in a test. Presumably the current implementation of toolz would fail this test.

[Digenis]

Yes. I hesitated committing because it doesn't make sense to me. It just checks this specific name, "func".
The test will not cover a case where someone introduces a positional argument with another name.
Instead, from what I implied in my previous comment, I would inspect the argspec in a test to verify that the signature names no positional arguments. Why not?

$ git log --oneline -4
f9e479d test_curry_kwargs: curry must not pick func kwarg
67e88dc coverage for curry() without func
d2570d4 Remove positional arg "func" from curry.__init__
0ad38b6 Merge pull request #216 from manishtomar/patch-1
$ git checkout 0ad38b6 -- toolz/functoolz.py                 
$ coverage run --source=toolz $(which nosetests) --with-doctest
    ...
======================================================================
ERROR: test_functoolz.test_curry_kwargs
----------------------------------------------------------------------
Traceback (most recent call last):
    ...
  File "/home/digenis/src/toolz/toolz/tests/test_functoolz.py", line 195, in test_curry_kwargs
    assert curry(h)(func=str)(0.0) == '0.0'
  File "/home/digenis/src/toolz/toolz/functoolz.py", line 224, in __call__
    return curry(self._partial, *args, **kwargs)
TypeError: __init__() got multiple values for keyword argument 'func'
    ...
FAILED (errors=1)

[Digenis]

OK, I figured what's funny with the build. I 'll need some time for it.

[eriknw]

This looks good to me. It appears the error is with pypy's version of functools.partial, which fails if passed func as a keyword.

The easiest way forward is to probably just skip the failing test if the platform is pypy. This can be done programmatically via platform.python_implementation() != 'PyPy'.

[Digenis]

Yes, this is what I figured too but I had no time to try fixing pypy and see if it still happens. Funny fact, pypy's partial (which breaks) has exactly the same issue, and the argument is also named func.

I will surely have the time to look into it next week.

[Digenis]

I sent a patch to pypy an hour ago.
I don't know when the latest version is going to reach travis.
Is it possible to trigger the build again sometime soon?

[Digenis]

I fixed the pypy bug months ago.
Even that bug was irrelevant to this PR, the code here is correct.
Is there a reason not having this merged?

[mrocklin]

Mostly the reason is that we check these things less frequently than we should. I've triggered a travis.ci build.

[mrocklin]

Looks like there is still a pypy3 bug. The CPython errors are pep8 issues.

[Digenis]

It is merged into pypy3

From the build I can see pypy2 2.5 succeeds
while pypy3 2.4 fails.

Travis doesn't have its latest version,
maybe pypy3 has a different release cycle
or it got the merge too late for the release.

(also: to rebase or not?)

[Digenis]

Do you want me to temporarily ignore the test for pypy3
to prevent the build from giving the wrong impression about the correctness of pytoolz??

[eriknw]

Yeah, that'd be great to skip the failing tests for pypy3. I'll merge as soon as the tests pass. Also, would you like to add yourself to AUTHORS.md?

Thanks for your persistence and patience--and for finding and fixing a bug in PyPy!

[Digenis]

I rebased because of a merge conflict with another contributor addition.
(Yet I still have a backup branch. Sorry for the mess)

[eriknw]

Very nice. Welcome to PyToolz!

[Digenis]

Thank you.
Maybe I'll reopen the same PR reverting 899a932 when PyPy3 releases 2.5

[Digenis]

just a reminder that 899a932 can be reverted