support adding per implementation warnings for hookspecs #138
Conversation
|
I like this a lot better than my potential proposal in #133 |
|
@justanr oh, i missed that, i just created this because i need it and because deprecation is a pretty concrete topic in some way (plus its nicer if you get warnings like that) - my implementation might even be a bit too concrete ^^ i hope for due scrutiny ^^ |
docs/index.rst
Outdated
|
|
||
| .. code-block:: python | ||
|
|
||
| @hookspec(impl_warning=DeprecationWarning("oldhook is deprecated and will be removed soon")) |
There was a problem hiding this comment.
What about just warning?
I don't know that the impl_ adds anything useful.
There was a problem hiding this comment.
warning alone is a overly generic name, thats why i added the impl in front - im happy about picking a more specific name thats describing what is happening, im severely opposed to going for a more generic name
its probably a good idea to brainstorm a better name, its fine if its a bit longer
There was a problem hiding this comment.
It might be general but it's as specific as the code you've written.
The type of warning is handed into the decorator and the underlying code merely uses the warnings mod to warn. Unless you intend to limit to specific subset of warning categories (which doesn't seem all that likely nor easy to describe in a variable name) I don't see how it can be more specific.
There was a problem hiding this comment.
@tgoodlet im not talking about types or calls - im talking about the intent of the value passed - which is a warning that is given for each implementation of the hook encountered - its bad style to encode types, its good style to enode intent
There was a problem hiding this comment.
Ok what about warning_on_call or warning_when_called?
I still think it's implicit that the warning is to be emitted at some point in the hook's lifetime and trying cram a more specific description in the variable name seems unnatural.
There was a problem hiding this comment.
the warnings are happening at registration time, not invocation time, so on call would be wrong
warning_on_impl or warning_on_implementation might be more to the point tho
There was a problem hiding this comment.
i'll sleep a night over it and see what the impression is
There was a problem hiding this comment.
i decided to call it warn_on_impl since we consistently use impl as shortcut for implementation
|
Looks mostly fine to me @RonnyPfannschmidt other then a nitpick at the name. |
RonnyPfannschmidt
force-pushed
the
hook-warn-defined
branch
from
764f7e0
to
a400a2c
Compare
this will be used later on in pytest to deprecate hooks we dont really use/support anymore so we can remove them in future
this can also be used to do futurewarnings for experimental hooks