Turned warnings into errors #89
Conversation
- Fix two correct warnings - Used pytest.warns() around a valid warning being issued Should remove the warning exception for pytest-dev#81 when we tackle that. Fix pytest-dev#78
| @@ -675,7 +675,8 @@ def __call__(self, *args, **kwargs): | |||
| warnings.warn( | |||
There was a problem hiding this comment.
Guys shouldn't we turn this into a error right away instead of just a warning? IIUC the effect of this is things silently working because the missing argument is not required by any of the impls, but as soon as a new plugin is registered then everything blows up.
There was a problem hiding this comment.
but as soon as a new plugin is registered then everything blows up.
Only if the hook is called with less then the spec's args and some hookimpl is trying to accept those new args.
That will happen anyway; this code makes no difference.
IIRC this was introduced is to alert hook callers of newly introduced args without enforcing they're provided in calls (yet). An example might be where a plugin supports hook calling multiple times but newer versions should be called with additional args. An example might be in pytest where you want to add an additional arg to say pytest_runtestloop but don't want to break anyone's code who implements pytest_cmdline_main and already calls that function themselves. IIRC this came from the initial attempt at solving #15 and I think I pulled this out and PR-ed it in separately. I think this came from the idea of "graceful introduction" of new arguments.
I'm all for scrapping it though if it's not making any sense.
@RonnyPfannschmidt your thoughts?
There was a problem hiding this comment.
Only if the hook is called with less then the spec's args and some hookimpl is trying to accept those new args.
That will happen anyway; this code makes no difference.
Indeed it will happen, but I would expect it to happen sooner (as soon as the new spec argument was introduced) than later (only after a hookimpl which needs it).
But now that we mention #15 it makes sense, because then the hookiml will receive the default value defined in the spec, instead of things blow up.
If this is a half-way measure to get there I think we are fine then.
Thanks for the explanation! 😉
There was a problem hiding this comment.
Indeed it will happen, but I would expect it to happen sooner (as soon as the new spec argument was introduced) than later (only after a hookimpl which needs it).
Right it would just break client code immediately then - which I'm also totally cool with.
Previously this was being ignored without the error anyway.
I wonder what @RonnyPfannschmidt thinks?
There was a problem hiding this comment.
breaking client code right away is not something that should be done right away if it can be avoided
change and breaking change is necessary to keep the world moving, but unexpected hard breakage should be avoided unless the cost of doing so is rather unreasonable
| filterwarnings = | ||
| error | ||
| # inspect.getargspec() ignored, should be fixed in #81 | ||
| ignore:inspect.getargspec().*deprecated |
There was a problem hiding this comment.
nice did not know you could do that 👍
Should remove the warning exception for #81 when we tackle that.
Fix #78