Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added more actions, and improved action handling. #5
Conversation
| for obj in objects: | ||
| - await func(model, obj, **kwargs) | ||
| - self[func.__name__] = wrapped | ||
| + await enforce.runtime_validation(func( |
| - self[func.__name__] = wrapped | ||
| + await enforce.runtime_validation(func( | ||
| + rule, model, obj, **kwargs)) | ||
| + self[func.__name__] = {'func': wrapped, 'args': func.__annotations__} |
bcsaller
Oct 21, 2016
Contributor
There are a number of ways this might be better to do. I am still quite wary of just matching names like we do here (and like I might do a few other places).
I'd suggest (even though it will have the same outcome) that we future proof this a little bit by using inspect.signature. The Signature object it returns should have all the function information we want.
| context.bus.dispatch( | ||
| origin="glitch", | ||
| - payload=functools.partial(actionf, model, objects, **action), | ||
| + payload={'action': actionf.__name__, **action}, |
bcsaller
Oct 21, 2016
Contributor
we might be better off here if this were the inspect.Signature object as well. It should handle things like the qualname vs name handling
petevg
Oct 24, 2016
Collaborator
Since this is strictly for logging, I think that I'm comfortable with trusting the __name__ for now. If we wind up passing the function through more wrappers and wind up mangling it, we can dig into it with inspect ...
| @@ -10,6 +10,49 @@ class InvalidModel(Exception): | ||
| pass | ||
| +_MODEL_OPS = { |
bcsaller
Oct 21, 2016
Contributor
I like this change, I do things we'll want a method for pushing new entries in from plugins. Maybe glitch can use a passed in copy of this in generate plan and all the plugins get called to see if they update that struct.
petevg
Oct 24, 2016
Collaborator
Passing this as an arg makes a lot of sense. I'd like to hold off on doing so for now, though -- without seeing a plugin that wants to interact with this dict, it's hard to tell where to put it. (Maybe it's a .yaml file that gets read in when we create the context.)
| - # TODO: get some good default args for each action | ||
| - 'selectors': selectors | ||
| - } | ||
| + sig = [arg for arg in Actions[action]['args'] if arg in _MODEL_OPS.keys()] |
bcsaller
Oct 21, 2016
Contributor
Like I mentioned I'd like something a little more formal than this convention here, but we can leave that as a TODO. I am not sure of the best path forward here yet.
| -class TestAction(unittest.TestCase): | ||
| - def test_define(self): | ||
| - self.assertFalse('faux_action' in Actions) | ||
| +#class TestAction(unittest.TestCase): |
| @@ -15,28 +15,15 @@ def setUp(self): | ||
| self.loop = asyncio.get_event_loop() | ||
| self.context = mock.Mock() | ||
| self.rule = mock.Mock() | ||
| + self.rule.log.debug = print |
bcsaller
Oct 21, 2016
Contributor
These methods don't have the same signature and can break when used in this way. We can add a test.utils that can set up a default handler that does this.
petevg
Oct 24, 2016
Collaborator
This whole test is really an integration test, and is getting increasingly unwieldy. Replacing it with a matrix test plan.
bcsaller
reviewed
Oct 21, 2016
Let me know what you think of the feedback, After this I think we can merge this. Thanks
petevg
added some commits
Oct 21, 2016
petevg commentedOct 20, 2016
Plan can now select from actions that have arguments other than a unit.
Cleaned up TODOs and generally made things better.
@johnsca @bcsaller