-
Notifications
You must be signed in to change notification settings - Fork 125
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
Drop multicall #147
Drop multicall #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Related to #59 btw.
it should be noted that we require a breaking pytest release to update to this one |
On pytest we probably will no longer pin pluggy after |
Same for tox, I guess :) |
@RonnyPfannschmidt yes @nicoddemus makes a good point: |
@tgoodlet @nicoddemus that ignores pytest possibly wanting to use new features of pluggy and the fact that pluggy is part of the public api of pytest, as such, break a part break the whole i'd like to be pedantic about things there |
Since pytest 6.0 is coming, maybe it's a good time to merge this, release pluggy 1.0 and require |
@bluetech I'm in. This has been getting put off for no real reason really. |
Drop the `_LegacyMultiCall` type and it's recursion "fun". We've got a faster and simpler function-loop approach now with `pluggy.callers._multicall()` it's been doing great in production! Resolves pytest-dev#59
Just working out some kinks from the rebase onto master. |
Ok think I got it all. One question, should I start a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some suggestions, other than that, LGTM!
@@ -38,7 +38,7 @@ def wrappers(request): | |||
return [wrapper for i in range(request.param)] | |||
|
|||
|
|||
@pytest.fixture(params=[_multicall, _legacymulticall], ids=lambda item: item.__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figure this fixture isn't really needed any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lel, good point.
@@ -14,34 +14,9 @@ def MC(methods, kwargs, firstresult=False): | |||
for method in methods: | |||
f = HookImpl(None, "<temp>", method, method.example_impl) | |||
hookfuncs.append(f) | |||
if "__multicall__" in f.argnames: | |||
caller = _legacymulticall | |||
return caller(hookfuncs, kwargs, firstresult=firstresult) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the caller
indirection anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woo yeah this should be a nice cleanup 😸
"removed in an upcoming release.", | ||
DeprecationWarning, | ||
) | ||
self.multicall = _legacymulticall |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the self.multicall
indirection is not needed anymore -- can call _multicall
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech actually, come to think of this I wonder if it makes sense to keep this in preparation for something like #50? The overloading of the multi-caller I guess can be done anywhere?
I'd like @RonnyPfannschmidt's opinion on this.
Also this might bring up discussion on whether the PluginManager._inner_hookexec
is something we could also get rid of since it only seems to be used for tracing iirc. Might be better addressed in a new issue/discussion tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, come to think of this I wonder if it makes sense to keep this in preparation for something like #50?
I'd say remove it and let #50 add it back, to show the "real cost", but up to you.
Also this might bring up discussion on whether the PluginManager._inner_hookexec is something we could also get rid of since it only seems to be used for tracing iirc
pytest seems to use the tracing stuff (add_hookcall_monitoring
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest seems to use the tracing stuff
Definitely, I've just wondered about implementing the tracing as part of the hook caller instead of some wrapper slapped in around the _hookexec()
by the PluginManager
.
Looking at this again I'm more convinced all that tracing magic should be implemented on the _HookCaller
including adding a method to do the .subset_hook_caller()
stuff. In particular it's currently limiting us from doing per-hook tracing as well.
If you want to do tracing around the caller I'm not sure why that has anything to do (abstraction wise) with the plugin manager - the only relation really is the global context of tracing all hooks that is currently supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech either way I dropped the _HookCaller.multicall
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made #262 regarding my 2nd last comment.
changelog/59.removal.rst
Outdated
@@ -0,0 +1,2 @@ | |||
Remove internal usage of legacy ``__multicall__`` recursive hook calling system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm misunderstanding, this is a public API, not just internal usage, so
Remove internal usage of legacy ``__multicall__`` recursive hook calling system. | |
Remove legacy ``__multicall__`` recursive hook calling system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluetech actually I'm pretty sure this was always an internal system used as a hack, which is why we were also able to remove it.
I think @RonnyPfannschmidt might be able to comment since he has seniority 😼
I think it would be good to have pre-release versions, we can change pytest master to use them. |
From the link you provided, looks like it should rather be I think alpha or beta is appropriate (compared to the "Developmental releases" which are described as regularly occurring things). |
@bluetech fwiw I'm pretty sure
Plus I think |
Setuptools_scm supports a .dev tag suffix to denote a new development epoch |
Just waiting on what tag I should push team 😄 |
i would propose 1.0.dev to be inserted on master after merge |
Just to get some eyes on it as we approach
1.0
!