Skip to content

Avoid TypeError in call_historic()#110

Closed
rmfitzpatrick wants to merge 1 commit into
pytest-dev:masterfrom
rmfitzpatrick:call_historic_proc_check
Closed

Avoid TypeError in call_historic()#110
rmfitzpatrick wants to merge 1 commit into
pytest-dev:masterfrom
rmfitzpatrick:call_historic_proc_check

Conversation

@rmfitzpatrick

Copy link
Copy Markdown

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

@goodboy

goodboy commented Dec 6, 2017

Copy link
Copy Markdown
Contributor

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

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

@nicoddemus

Copy link
Copy Markdown
Member

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

This would warrant a 0.7 release though.

@RonnyPfannschmidt

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

goodboy commented Dec 7, 2017

Copy link
Copy Markdown
Contributor

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?

@nicoddemus nicoddemus left a comment

Copy link
Copy Markdown
Member

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

goodboy commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

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

@goodboy

goodboy commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

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