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.

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

Warn if "observe" decorator applied to method with only "self" #1428

Closed
mdickinson opened this issue Feb 1, 2021 · 4 comments · Fixed by #1529
Closed

Warn if "observe" decorator applied to method with only "self" #1428

mdickinson opened this issue Feb 1, 2021 · 4 comments · Fixed by #1529

Comments

@mdickinson
Copy link
Member

With the new 'observe' decorator, there's a potentially late-discovered bug that I've seen now in multiple different projects, and that's to forget to add the "event" argument on a previously argument-less on_trait_change target.

It may be worth warning if the target of the "observe" decorator does not have exactly one positional argument in addition to "self".

@kitchoi
Copy link
Contributor

kitchoi commented Feb 1, 2021

One thing to watch out is that the inspection on callable arguments adds limitations on what kind of callables can be used.
The fact that on_trait_change does signature inspection means a whole bunch of callables cannot be used:

>>> from traits.api import *
>>> class Foo(HasStrictTraits):
...     name = Str()
... 
>>> foo = Foo()
>>> events = []
>>> foo.on_trait_change(events.append, "name")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traits/has_traits.py", line 2557, in on_trait_change
    self._on_trait_change(
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traits/has_traits.py", line 2249, in _on_trait_change
    wrapper = self.wrappers[dispatch](handler, notifiers, target)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traits/trait_notifiers.py", line 412, in __init__
    self.init(handler, owner, target)
  File "/Users/kchoi/Work/ETS/py39-venv/traits-env/lib/python3.9/site-packages/traits/trait_notifiers.py", line 447, in init
    arg_count = handler.__code__.co_argcount
AttributeError: 'builtin_function_or_method' object has no attribute '__code__'

Currently this works:

>>> foo.observe(events.append, "name")
>>> foo.observe(print, "name")

Calling inspect.signature on certain built-in functions will also fail. If one detects if a function is an instance method first then subsequent signature inspection should be okay.

@mdickinson
Copy link
Member Author

Yes: I think it would be a warning, not an error, and we'd only issue the warning in the case that we could inspect the code - if we couldn't inspect the code we'd just let it pass.

@mdickinson
Copy link
Member Author

Having implemented a solution, I'm having second thoughts about this; it's a bit inefficient to be doing runtime type-checking for this. A static type-check may be a better solution. If we get the signature of observe right, presumably mypy could do such a static check for us.

@mdickinson
Copy link
Member Author

Third thoughts: I think this is fine for the observe decorator factory, which is where I've seen the issue most often, but would hesitate to extend the check to uses of the observe method. PR #1529 only solves this for the decorator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants