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.

Sign up for GitHub

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

Jump to bottom

Turned warnings into errors #89

Merged
merged 1 commit into from
Sep 13, 2017
Merged

Turned warnings into errors #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pluggy/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,8 @@ def __call__(self, *args, **kwargs):
warnings.warn(
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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! 😉

Copy link
Contributor

@goodboy goodboy Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

"Argument(s) {0} which are declared in the hookspec "
"can not be found in this hook call"
.format(tuple(notincall))
.format(tuple(notincall)),
stacklevel=2,
)
return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)

Expand Down
4 changes: 2 additions & 2 deletions testing/test_method_ordering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def after(outcome, hook_name, hook_impls, kwargs):

undo = he_pm.add_hookcall_monitoring(before, after)

he_pm.hook.he_method1()
he_pm.hook.he_method1(arg=1)
assert len(l) == 4
assert l[0][0] == "he_method1"
assert len(l[0][1]) == 2
Expand All @@ -249,7 +249,7 @@ def after(outcome, hook_name, hook_impls, kwargs):
assert l[3][1] == l[0][0]

undo()
he_pm.hook.he_method1()
he_pm.hook.he_method1(arg=1)
assert len(l) == 4 + 2


Expand Down
3 changes: 2 additions & 1 deletion testing/test_pluginmanager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ def he_method1(self, arg):
0 / 0
pm.register(Plugin1())
with pytest.raises(HookCallError):
pm.hook.he_method1()
with pytest.warns(UserWarning):
pm.hook.he_method1()


def test_subset_hook_caller(pm):
Expand Down
4 changes: 4 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ minversion=2.0
#--pyargs --doctest-modules --ignore=.tox
addopts=-rxsX
norecursedirs=.tox ja .hg .env*
filterwarnings =
error
# inspect.getargspec() ignored, should be fixed in #81
ignore:inspect.getargspec().*deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

nice did not know you could do that 👍


[flake8]
max-line-length=99