Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically handle different signatures in Callable #1260

Merged
merged 24 commits into from Apr 10, 2017

Conversation

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Apr 10, 2017

This PR aims to address #1189. The goal is to make Callable very flexible in what it accepts when it dispatches args and kwargs to the callback. This should mean that DynamicMap can accept almost any sensible callback.

Action items

  • Make Callable flexible in how it dispatches arguments
  • Validate argspec against kdims and streams in DynamicMap
  • Add more unit tests
methods, classmethods and partials.
Note that the args list for instance and class methods are those as
seen by the user. In other words, the first argument with is

This comment has been minimized.

@jbednar

jbednar Apr 10, 2017
Member

s/with/which?

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017
Author Contributor

Thanks. Fixed in 51a1016

@jbednar
Copy link
Member

@jbednar jbednar commented Apr 10, 2017

Thanks for doing this!

kwargs = dict(flattened, **dict(zip(self._posarg_keys, args)))
args = ()
else:
kwargs = dict(flattened)

This comment has been minimized.

@philippjfr

philippjfr Apr 10, 2017
Member

No reason to have this inside the context manager as far as I can tell.

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017
Author Contributor

Agreed. I just did this because I wanted to be close to the actual call when I wrote it.

This comment has been minimized.

@jlstevens

jlstevens Apr 10, 2017
Author Contributor

Addressed in 50022df

@philippjfr

This comment has been minimized.

Copy link
Member

@philippjfr philippjfr commented on tests/testcallable.py in 5ec7a2b Apr 10, 2017

"does not"?

This comment has been minimized.

Copy link
Contributor Author

@jlstevens jlstevens replied Apr 10, 2017

Fixed in f5c370c

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented Apr 10, 2017

I think this is ready to merge once the tests pass. As ever, the new functionality needs to be documented in the tutorials, but that is outside the scope of this PR.

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Apr 10, 2017

As ever, the new functionality needs to be documented in the tutorials, but that is outside the scope of this PR.

Agreed, but in the future after we've released 2.0 and overhauled our docs, I'd say that every feature PR should update be documented as a pre-requisite for being merged.

@jlstevens
Copy link
Contributor Author

@jlstevens jlstevens commented Apr 10, 2017

That would be a good policy for the future and we should codify it at some point (e.g when we decide on a PR template).

Right now it doesn't make sense as the tutorials relating to DynamicMap need a complete overhaul.

@philippjfr
Copy link
Member

@philippjfr philippjfr commented Apr 10, 2017

Looks good, those unit tests cover all the cases I can think of so I'm happy to merge.

@philippjfr philippjfr merged commit dfd92ad into master Apr 10, 2017
4 checks passed
4 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 78.985%
Details
@philippjfr
s3-reference-data-cache Test data is cached.
Details
@philippjfr philippjfr deleted the callable_autosignature branch Apr 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants