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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/with/which?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed in 51a1016

@jbednar
Copy link
Member

jbednar commented Apr 10, 2017

Thanks for doing this!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 50022df

@jlstevens
Copy link
Contributor Author

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

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

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

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
@philippjfr philippjfr deleted the callable_autosignature branch April 11, 2017 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants