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

Projects
None yet
3 participants
@jlstevens
Member

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

@jlstevens jlstevens added the WIP label Apr 10, 2017

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

Member

Thanks. Fixed in 51a1016

@jbednar

This comment has been minimized.

Member

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

Member

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

Member

Addressed in 50022df

@philippjfr

This comment has been minimized.

Member

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

"does not"?

This comment has been minimized.

Member

jlstevens replied Apr 10, 2017

Fixed in f5c370c

@jlstevens jlstevens added the ready label Apr 10, 2017

@jlstevens jlstevens removed the WIP label Apr 10, 2017

@jlstevens

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

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

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
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the callable_autosignature branch Apr 11, 2017

@jlstevens jlstevens referenced this pull request Apr 17, 2017

Merged

Improve DynamicMap usability and deprecate sampled mode #1305

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