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

Avoid TypeError in call_historic() #110

Closed
wants to merge 1 commit into from
Closed

Avoid TypeError in call_historic() #110

wants to merge 1 commit into from

Conversation

rmfitzpatrick
Copy link

The default argument for proc in _HookCaller.call_historic() is None. No attempt should be made to call it.

@goodboy
Copy link
Contributor

goodboy commented Dec 6, 2017

Heh, thanks @rmfitzpatrick!

@RonnyPfannschmidt @nicoddemus I think we need some tests to cover this API. Can you guys point me to where this proc is even getting used traditionally - I'm assuming it's pytest-xdist? Maybe we should be decoupling the ad-hoc naming for this (proc: process or processor whaa?) and formalizing an API for how this is intended to be used (say maybe a callback kwarg) and documenting it?

@nicoddemus
Copy link
Member

The only place I could find that uses is the call of the pytest_namespace hook:

        def do_setns(dic):
            import pytest
            setns(pytest, dic)

        self.hook.pytest_namespace.call_historic(do_setns, {})
        self.hook.pytest_addoption.call_historic(kwargs=dict(parser=self._parser))

You are right, callback probably would be a better name for it. I'm OK with changing it to a keyword-only argument and documenting it.

@RonnyPfannschmidt
Copy link
Member

callback is a bad name - result_callback as at least needed to convey meaning - this processes results

@nicoddemus
Copy link
Member

Agree that result_callback is an even better name than callback. 👍

This would warrant a 0.7 release though.

@RonnyPfannschmidt
Copy link
Member

the old and the new name can be supported simultaneous, the old name should trigger a deprecation and we need pytest release as well

@goodboy
Copy link
Contributor

goodboy commented Dec 7, 2017

Agree that result_callback is an even better name

Fair but what else would the callback be for ;P - either way I like explicit so result_callback works for me.

@rmfitzpatrick Do you mind adding a small test which would have caught this bug without your patch?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Please add a test to avoid future regressions. 🙇

goodboy pushed a commit to goodboy/pluggy that referenced this pull request Jan 10, 2018
Ensure that if a result callback (dubbed `proc` for the moment)
provided to `PluginManager.call_historic()` is `None`, no error occurs.

Relates to pytest-dev#110
@goodboy
Copy link
Contributor

goodboy commented Jan 10, 2018

@nicoddemus I've gone and added the requested test in #119.
Once this is merged that test should pass.

@goodboy
Copy link
Contributor

goodboy commented Jan 10, 2018

Closing in favour of #119.

@goodboy goodboy closed this Jan 10, 2018
goodboy pushed a commit to goodboy/pluggy that referenced this pull request Jan 10, 2018
Ensure that if a result callback (dubbed `proc` for the moment)
provided to `PluginManager.call_historic()` is `None`, no error occurs.

Relates to pytest-dev#110
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

Successfully merging this pull request may close these issues.

4 participants