Create feedback decorator #66
Conversation
magicbot/magicrobot.py
Outdated
| @@ -540,6 +560,11 @@ def _inject(self, n, inject_type, cname, component): | |||
| # for f in d.keys(): | |||
| # d[f] = f(component) | |||
|
|
|||
| def update_feedback(self): | |||
There was a problem hiding this comment.
Why isn't anything calling this?
There was a problem hiding this comment.
Because I don't know how magicrobot works 😉
| @@ -17,6 +17,21 @@ | |||
|
|
|||
| __all__ = ['MagicRobot'] | |||
|
|
|||
| def feedback(key=None): | |||
There was a problem hiding this comment.
No docstring? What is this supposed to do? Maybe add an example in the docstring too?
magicbot/magicrobot.py
Outdated
| # If no key is passed, get key from method name. | ||
| # -1 instead of 1 in case 'get_' is not present, | ||
| # in which case the key will be the method name | ||
| nt_key = func.__name__.split('get_')[-1] |
There was a problem hiding this comment.
This will break if get_ is a non-prefix substring of the function name.
There was a problem hiding this comment.
How so? It should still return whatever comes after 'get_', no?
There was a problem hiding this comment.
>>> 'forget_me'.split('get_')[-1]
'me'There was a problem hiding this comment.
In that scenario, I imagine you want forget_me to be the method, but what about if the method name was... reverse_get_speed. Would the expected key be speed? or reverse_get_speed
There was a problem hiding this comment.
Another problem is what if they use camelCase and not lowercase_underscore?
magicbot/magicrobot.py
Outdated
| def _update_feedback(self): | ||
| for (component, cname, name) in self._feedbacks: | ||
| # Put ntvalue at /robot/components/component/key | ||
| self.__nt.putValue('/components/{0}/{1}'.format(cname, getattr(component, name).__key__), getattr(component, name)()) |
There was a problem hiding this comment.
Maybe avoid calling getattr with the same arguments multiple times. getattr(...)() looks particularly ugly.
| setattr(func, '__feedback__', True) | ||
| # Store key within the function to avoid using class dictionary | ||
| setattr(func, '__key__', nt_key) | ||
| return func |
There was a problem hiding this comment.
I feel that this should check the function to see if (a) it's a function and (b) do the function arguments match what is required (eg, only a single argument named 'self').
There was a problem hiding this comment.
Why does it need to be named self?
There was a problem hiding this comment.
It doesn't need to be but there typically aren't reasons for it not to be, so we should enforce it.
There was a problem hiding this comment.
Luckily state_machine has some code I can steal
There was a problem hiding this comment.
Hm I thought I did...
There was a problem hiding this comment.
Lol I did just didn't push
magicbot/magicrobot.py
Outdated
| for (component, cname, name) in self._feedbacks: | ||
| func = getattr(component, name) | ||
| # Put ntvalue at /robot/components/component/key | ||
| self.__nt.putValue('/components/{0}/{1}'.format(cname, func.__key__), func()) |
There was a problem hiding this comment.
If func throws, then the entire robot will be dead.
There was a problem hiding this comment.
That'd be embarrassing
Not sure how this would get triggered though, since the name only gets added to the feedbacks if it is there with the feedback decorator, but, better safe than sorry
I've decided that (as the docstring states), if the method doesn't start with `get_` then the key in NetworkTables will be the full name of the method
magicbot/magicrobot.py
Outdated
| nt_key = func.__name__.split('get_')[-1] | ||
| name = func.__name__ | ||
| if name.startswith('get_'): | ||
| nt_key = name.split('get_')[1] |
There was a problem hiding this comment.
This will also still fail if get_ is a non-prefix substring of name.
There was a problem hiding this comment.
No, it won't. That's what the startswith checks for
Oh you mean if it starts with get and has get again...hmmm...and ideas on how to deal with?
The first iteration of a feedback decorator.
I think paired with an auto-send property that you can use with variables and not methods, networktables will be much smoother