Raise when unknown signals are connected to CallbackRegistries.#21238
Raise when unknown signals are connected to CallbackRegistries.#21238tacaswell merged 1 commit intomatplotlib:mainfrom
Conversation
anntzer
force-pushed
the
callbackrestrict
branch
from
9fbcd13 to
a39026a
Compare
anntzer
force-pushed
the
callbackrestrict
branch
from
a39026a to
3c3f164
Compare
anntzer
force-pushed
the
callbackrestrict
branch
from
3c3f164 to
f22d358
Compare
| signals : list, optional | ||
| If not None, *signals* is a list of signals that this registry handles: | ||
| attempting to `process` or to `connect` to a signal not in the list | ||
| throws a `ValueError`. The default, None, does not restrict the | ||
| handled signals. |
There was a problem hiding this comment.
This is a very useful addition. Not being able to see easily which signals are available on a registry has always been a source of confusion and uncertainty to me.
Is there a reason signals is optional (other than backward compatibility)? If not, I propose to deprecate not specifying signals.
There was a problem hiding this comment.
backend_tools (MEP22) currently relies on being able to dynamically add signals. I'm not actually convinced that it's a good idea, but that's another discussion to have some other time...
There was a problem hiding this comment.
Can we have a private _register_signal() method for that case? Since CallbackRegistry is technically a public API, I'm in favor of changing the signal API only once.
There was a problem hiding this comment.
@tacaswell was also against that (#8839 (comment)). I personally don't have a very strong opinion either way, though (well, I think the whole CallbackRegistry API could be revisited, but that's another discussion :-)).
There was a problem hiding this comment.
I'm not following the "not knowing is a feature" argument. It's comparable to adding attributes at runtime. It can be helpful in very specific cases, but in general it's rather confusing to not know the capabilities of an object.
Anyway, it's not important enough that I would block. If others want to approve as is that's fine for me.
There was a problem hiding this comment.
I mostly stand by my comment (I do not always agree with myself from 2017!), but I agree that not knowing what keys are supported can be very confusing and I really like the analogy to runtime adding attributes.
I would like to preserve the ability to have a any-to-any registry, I think having a "like known and warn on unknown" behavior is a bad mixed-state with the worst of both worlds.
I am in favor of making signals mandatory eventually with the ability to pass None to get the naive dispatcher.
I think adding a (public) API to manage the known signals is also a good idea.
I chose not to emit warnings during a deprecation period as otherwise the semantics of the `signals` parameter would again have to change at the end of the deprecation period (from warning to raising), which would be another backcompatibility break...
anntzer
force-pushed
the
callbackrestrict
branch
from
f22d358 to
7f5690e
Compare
|
@dstansby you originally suggested this be fixed - any bandwidth to review this? Thanks! |
timhoffm
left a comment
timhoffm
left a comment
There was a problem hiding this comment.
I still think that signals should be mandatory in the long run, but as a first step and considering compatibility, starting optional is ok.
|
Merged to keep the perfect of being the enemy of the good. |
|
Thanks for this! |
6 participants
I chose not to emit warnings during a deprecation period as otherwise
the semantics of the
signalsparameter would again have to change atthe end of the deprecation period (from warning to raising), which would
be another backcompatibility break...
Closes #8839.
PR Summary
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).