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

Deprecate __multicall__ support #23

Closed
goodboy opened this issue Sep 13, 2016 · 9 comments
Closed

Deprecate __multicall__ support #23

goodboy opened this issue Sep 13, 2016 · 9 comments
Assignees

Comments

@goodboy
Copy link
Contributor

goodboy commented Sep 13, 2016

As per the comment in pluggy._MultiCall we should be able to drop the line here. Grepping the pytest sources reveals that only a few spots still reference it.

This change also allows us to remove the recursion induced by _wrapped_call() getting passed _MultiCall.execute. This means we can remove _wrapped_call and simply loop through hookwrapper pre and post yield calls at the beginning and end of execute(). I'd actually be interested to see if contextlib.contextmanager wrapping could be used directly for this.

@RonnyPfannschmidt
Copy link
Member

it exists solely for backward compatibility

at the pytest sprint we discussed that instead of just removing it,
we should have a separate faster implementation without it and choose based on the hook

at the pytest sprint we discussed that instead of just removing it,
we should have a separate faster implementation without it and choose based on the hooks

@goodboy
Copy link
Contributor Author

goodboy commented Sep 13, 2016

@RonnyPfannschmidt ok so if I implement this where do you want the check for __multicall__ support to be?

@RonnyPfannschmidt
Copy link
Member

i dont care where it is ^^ it has to work reliable :)

@hpk42
Copy link
Contributor

hpk42 commented Sep 14, 2016

I guess it makes sense to check if multicall is used during registering a plugin and mark the hook caller accordingly. On instantiation of PluginManager one should be able to optionally specify a callback which receives information about plugin and hook which uses multicall so a warning can be added.

Then at hook calling time we can switch between the two implementations. The current one would be used for when multicall is used.

@hpk42
Copy link
Contributor

hpk42 commented Nov 11, 2016

Also it makes sense to get rid of the last pytest usages of __multicall__ instances besides.

@RonnyPfannschmidt
Copy link
Member

we should probably put the deprecation-warning into pluggy using python warnings themselfes
as for pytest itself, a quick grin showed that only the testsuite has a few useages leftover

@nicoddemus
Copy link
Member

Oh really? I swear I have removed those. 😬

@goodboy
Copy link
Contributor Author

goodboy commented Nov 11, 2016

Gonna try and hack on this over the weekend.

@goodboy
Copy link
Contributor Author

goodboy commented Feb 16, 2017

@hpk42 I actually started this but it turns out the way that tracing is accomplished kind of makes it a bit tricky with _MultiCall being statically assigned in the PluginManager._inner_hookexec lambda.

So I wanted to propose:

  • each _HookCaller is instantiated with it's own _MultiCall and we revert to a LegacyMultiCall only when '__multicall__' is detected in a hookimpl signature (i.e. when _HookCaller._add_hookimpl() is called)
  • make tracing enabled per _HookCaller instead of globally
    • we can still keep the PluginManager.enable_tracing() method (which could now also accept an optional hookname arg) that does it globally but internally it will enable tracing on each _HookCaller.
  • this should let us get rid of the confusing indirection of PluginManager._hookexec() and _inner_hookexec because multicall objects will be instantiated once per _HookCaller instead of on every hook call.

Let me know what you think.

@goodboy goodboy self-assigned this Apr 22, 2017
This was referenced Jul 5, 2017
@goodboy goodboy changed the title Drop __multicall__ support Deprecate __multicall__ support Jul 9, 2017
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

No branches or pull requests

4 participants