Added ability to Actions to add actions after init #862
Conversation
| @@ -154,7 +154,11 @@ class Actions(object): | |||
| def __init__(self, *args, **kwargs): | |||
| self.actions = None | |||
| self.args = args | |||
| self.kwargs = kwargs | |||
| self.kwargs = OrderedDict(kwargs) | |||
There was a problem hiding this comment.
What's the reason for the OrderedDict? It's perhaps a little misleading because **kwargs is not guaranteed to maintain order, so I'm not sure how you expect to use the ordering.
There was a problem hiding this comment.
Because dicts are handled differently in Python 2 and Python 3, I used an OrderedDict so that I could easily write a tests that would pass for both and so that the outcome would be the same regardless of Python version
There was a problem hiding this comment.
It's probably good to remember what order people added actions in, so I support the use of OrderedDict. If that's something that's going to effect the model that's built, then we definitely want to ensure it's the same between Python versions.
There was a problem hiding this comment.
Ah, looking at the last commit, I see now. One thing worth noting is it doesn't have as much to do with the Python version as it has to do with the nature of undefined behaviour. You could reorganize the code slightly (maybe change the name of the keyword arg) and then suddenly you could get the flip behaviour of it working in the opposite version but not the other (or both, or neither), or even failing on different machines.
At any rate, the comment about it being misleading is still a concern. To be clear about what I mean...
actions.add(a=x, b=y)
could have the order ('a', 'b') in some cases, and the order ('b', 'a') in other cases, due to the undefined ordering of **kwargs. The order will only be preserved under the condition that len(kwargs) <= 1.
IMO, making it a normal dict again might be the way to go, to avoid having a user rely on the behaviour only to inevitably encounter a weird issue like the above example. To make the tests work, you could do something like sort the actions in the test?
There was a problem hiding this comment.
Yes, very good point. This is a problem in add, too.
There was a problem hiding this comment.
That said, this was a problem before, too. At least this change makes things a bit better, but we should definitely put a comment in that things are not as ordered as they look. There's a PEP request for ordered kwargs, but without something like that, I don't think we can get the ordering without changing the syntax. The other option is to sort the keys (either as we add them, or when we're building the model), so that at least it's deterministic, if not in the same order that the user added them.
There was a problem hiding this comment.
I think I'm going to got with @hunse suggestion of sorting it alphabetically before processing. That way, we have deterministic behaviour can tested and if for some bizarre reason the user does want an oddly specific order, they can just prepend their kwargs with an index.
There was a problem hiding this comment.
Yeah that sounds good. The main idea is it should be all-or-nothing. So either clearly unordered or clearly ordered, otherwise people come to rely on something that seems defined, only to have things confusingly fall apart later when it gets more complex and difficult to debug. 👍
|
Fixed all the aforementioned issues. |
| @@ -168,7 +179,9 @@ def process(self, spa): | |||
| sources = list(spa.get_module_outputs()) | |||
| sinks = list(spa.get_module_inputs()) | |||
|
|
|||
| sorted_kwargs = OrderedDict(sorted(self.kwargs.items())) | |||
There was a problem hiding this comment.
Nit: you should be able to just do sorted(self.kwargs.items()) and then remove the iteritems on line 186. The OrderedDict part is redundant.
|
LGTM! |
|
Try rebasing to master and pushing again to fix the Travis issue? |
|
I'm in the process of merging, so I'll rebase shortly. |
|
Merge halted: realized this should have a changelog entry too. Added one in 9f1db3f; can someone look it over? |
|
Changelog looks good. |
|
👍 |
Resolves #861