Repository navigation
[ports] Add default values to PortInformation - #510
Conversation
laibazahid26
left a comment
laibazahid26
left a comment
There was a problem hiding this comment.
Two small questions left inline, both about the default value edge cases rather than the design itself.
| self.assertEqual(cons.get_input("items"), [1, 2]) | ||
| self.assertEqual(DefaultingConsumer.input_ports()["items"].default_value, [1, 2]) |
There was a problem hiding this comment.
question: input_ports() rebuilds the dict and the PortInformation objects on every call, so lines 314 and 315 each get a fresh [1, 2]. I think both assertions would pass with the copy.deepcopy removed from get_input().
Is a port declaration ever shared across instances, or is it always reconstructed? If it can be shared, keeping the dict at the class level in DefaultingConsumer would make this test actually exercise the copy. If not, the deepcopy may not be necessary at all.
There was a problem hiding this comment.
This is a great question -- in this particular case you're right that the deepcopy is unnecessary, but I didn't realize that this is set up so the dictionary keeps getting recreated. I would actually prefer having the deepcopies of the port values, but turning these methods into class attributes instead.
class MyBehaviour(PortsMixin, py_trees.behaviour.Behaviour):
INPUT_PORTS = {"input": PortInformation(data_type=str, required=True)}
OUTPUT_PORTS = {"output": PortInformation(data_type=str, required=True)}@JenniferBuehler what do you think? It's a breaking change but prevents unnecessary dictionary recreations every time we try to get the input port information.
Definitely for a follow-on PR though!
| """An explicit default= argument wins over the one declared on the port.""" | ||
| cons = DefaultingConsumer("cons") | ||
| cons.setup_ports() | ||
| self.assertEqual(cons.get_input("input", default="call_site"), "call_site") |
There was a problem hiding this comment.
question: get_input() uses None as the sentinel for “no argument default.” As a result, a falsy default falls through to the declared value. For example, get_input("input", default="") returns the declared value rather than "".
ports.rst says that “a call-site default still wins,” which is maybe stronger than the current implementation? Is that intentional? A module level _UNSET sentinel would allow False, 0, and "" to be used as explicit defaults. Either way, it could be useful for this test to include a falsy default case.
There was a problem hiding this comment.
I checked this and this is (thankfully) not the case, as all the checks are explicitly done with is not None rather than assuming falsiness.
For example, if we modify this exact like to do:
self.assertEqual(cons.get_input("input", default=""), "")it actually succeeds as expected, and does not fall back to the declared value.
Adds a
default_valueattribute to thePortInformationdataclass, which is a convenient extension that allows specifying default input/output port values right in the port declarations.Closes #489