Skip to content

Conversation

@ariebovenberg
Copy link

@ariebovenberg ariebovenberg commented May 22, 2018

Implements #397

some remarks:

  • I've also implemented __hash__, under the assumption that Compose objects are not intended to be mutated. Perhaps it is best to make this explicit, or to remove __hash__.
  • I have extended the current test method for compose. Perhaps it is best to split the test function up, considering its length.
  • do there need to be any additions to the docs?
  • I've implemented __signature__ for python3 only. Perhaps an elegant way of supporting python 2 is to import Signature from the funcsigs module, if it is installed. This way funcsigs is an optional dependency, keeping toolz itself lightweight and dependency-less.

edit: formatting

@ariebovenberg ariebovenberg changed the title improvements to "compose" [WIP] improvements to "compose" May 23, 2018
@ariebovenberg
Copy link
Author

I am getting a test failure on python 3.3. Not sure how to resolve this:

======================================================================
FAIL: test_inspect_args.test_introspect_builtin_modules
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python3.3.6/lib/python3.3/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/travis/build/pytoolz/toolz/toolz/tests/test_inspect_args.py", line 443, in test_introspect_builtin_modules
    raise AssertionError(message + '\n\n'.join(messages))
AssertionError: Missing introspection for the following callables:
toolz.functoolz:
    Compose

@eriknw
Copy link
Member

eriknw commented May 24, 2018

@ariebovenberg, thanks for following through and taking this on! I'll make time to give this a proper review soon.

Regarding the failure in Python 3.3, my guess is it's from __wrapped__ being defined. If you remove this, does the test pass?

We should probably drop support for Python 2.6 and 3.3 anyway (heh, it's a little funny/embarrassing that we still test and support these versions). I recall these versions (and even 3.4) caused some difficulty for me in the past, which is probably the main reason this work hasn't been done yet. I think inspection has stabilized for Python 3.5-3.7.

@ariebovenberg
Copy link
Author

@eriknw, removing __wrapped__ does fix the test on python 3.3.

Dropping support for python 2.6 and 3.3 makes sense though, as they are both officially EOL.

@ariebovenberg ariebovenberg changed the title [WIP] improvements to "compose" Improvements to "compose" May 29, 2018
@eriknw
Copy link
Member

eriknw commented Jul 6, 2018

Hey, I've been afk for a few weeks. At a glance, this PR looks pretty good. I have a lot to catch up on, but I'm looking forward to giving this a more detailed review.

These were deleted from the pytoolz GitHub account.
eriknw and others added 6 commits December 6, 2018 09:58
Fix DeprecationWarning in Python 3.7
fix memory leak with objects contained in the exception / traceback.
fix memory leak from exceptions
Fix a link and a formatting in a note on `map` and `filter`.
eriknw and others added 18 commits June 22, 2019 10:41
Update streaming-analytics.rst
DOC/BUG: fix import so in memory split-apply-combine example runs
Add hint for groupby to documentation
This involved some refactoring of the tests for compose(), so that
we can programmatically ensure that compose() and pipeline()
have equivalent tests.
This unfortunately removes ability of parametrized tests to fail
independently, but is necessary so that CI tests will pass
@eriknw eriknw merged commit c9d69b9 into pytoolz:master Jul 9, 2019
@eriknw
Copy link
Member

eriknw commented Jul 9, 2019

This is in! I took care of a merge conflict. Thanks @ariebovenberg!

@eriknw
Copy link
Member

eriknw commented Jul 9, 2019

Quick question: why did you define __wrapped__ attribute? When will this be used, and is it even valid given that compose is generally comprised of many functions?

@ariebovenberg
Copy link
Author

@eriknw it's so that you can use compose to make decorators, which will then have a propper __wrapped__ set.

def int_output(func):
     return compose(func, int)

@int_output
def mult(a, b):
     return a * b

It's fine to remove __wrapped__ if you don't want to support this.

@ariebovenberg
Copy link
Author

It's probably best to either remove __wrapped__, or comment/document why it is defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.