Add args and kwargs support to method execution #69
Conversation
|
Looks sane and useful. Will run tomorrow against some devices to make sure nothing broke. |
|
Instead of ignoring malformed yaml, should we be raising parser errors if one tries to pass in a list to kwargs, or a dictionary to args? |
napalm_yang/parser.py
Outdated
| for p in m["method"].split("."): | ||
| attr = getattr(attr, p) | ||
| r = attr(**m["args"]) | ||
| if isinstance(m.get("args", None), list): |
There was a problem hiding this comment.
shouldn't we default to an empty list/dict?
There was a problem hiding this comment.
defaults are set in lines 59/60 -- we just check if m has a valid dict/list to overwrite the default
There was a problem hiding this comment.
Ok, I see... I suggest doing this instead:
kwargs = m.get("kwargs", {})
if not isinstance(kwargs, dict):
raise TypeError("kwargs has to be a dict")
Otherwise it seems like you are just ignoring invalid arguments without providing any feedback to the user.
| @@ -183,13 +183,15 @@ the device. For example:: | |||
| parser: XMLParser | |||
| execute: | |||
| - method: _rpc | |||
| args: | |||
| args: [] | |||
There was a problem hiding this comment.
I don't think we need args. kwargs is probably enough.
There was a problem hiding this comment.
Both don't need to be specified in the YAML, but I think it might be wise to have the support in the future for args in the backend.
|
Sorry, got sidetracked, will try to test later this week. |
| if not isinstance(args, list): | ||
| raise TypeError("args must be type list, not type {}".format(type(args))) | ||
| kwargs = m.get("kwargs", {}) | ||
| if not isinstance(args, dict): |
There was a problem hiding this comment.
args should be kwargs but don't worry, I will fix it and merge via #70
There was a problem hiding this comment.
This is what I get for copy/paste
Noticed that there wasn't a way to run a method that took no arguments. Updated parse.py to support *args, **kwargs format.
Further testing probably required.