feat(api): Allow recovering from errors that happen during the preparation part of an aspirate command #16896
Conversation
| if isinstance(move_result, DefinedErrorData): | ||
| return DefinedErrorData(move_result.public, state_update=state_update) | ||
|
|
||
| # TODO: Figure out what to do for state_update_if_false_positive |
There was a problem hiding this comment.
imo passthrough and union with previous successes
There was a problem hiding this comment.
I think I've settled on ignoring state_update_if_false_positive until/unless this command actually uses it for something, in which case the right thing to do will hopefully be more obvious.
The reason I'm not setting up a pattern of "passthrough and union with previous successes" (yet) is just uncertainty on my part. Like, it kind of smells to me like an Implementation.execute() concern that our inner helper functions aren't in a good position to anticipate. And I wouldn't want to go out of our way to set up a pattern that does nothing now and starts doing the wrong thing later on.
SyntaxColoring
changed the title
| def append(self, other: Self) -> Self: | ||
| """Apply another `StateUpdate` "on top of" this one. | ||
|
|
||
| This object is mutated in-place, taking values from `other`. | ||
| If an attribute in `other` is `NO_CHANGE`, the value in this object is kept. | ||
| """ | ||
| fields = dataclasses.fields(other) | ||
| for field in fields: | ||
| other_value = other.__dict__[field.name] | ||
| if other_value != NO_CHANGE: | ||
| self.__dict__[field.name] = other_value | ||
| return self | ||
|
|
There was a problem hiding this comment.
Is append() a good name for this? Is there a naming scheme that harmonizes better with reduce()?
There was a problem hiding this comment.
you could make this reduce(), continue to take N arguments, and have self as a silent first argument. that is probably the most natural.
| def append(self, other: Self) -> Self: | ||
| """Apply another `StateUpdate` "on top of" this one. | ||
|
|
||
| This object is mutated in-place, taking values from `other`. | ||
| If an attribute in `other` is `NO_CHANGE`, the value in this object is kept. | ||
| """ | ||
| fields = dataclasses.fields(other) | ||
| for field in fields: | ||
| other_value = other.__dict__[field.name] | ||
| if other_value != NO_CHANGE: | ||
| self.__dict__[field.name] = other_value | ||
| return self | ||
|
|
There was a problem hiding this comment.
you could make this reduce(), continue to take N arguments, and have self as a silent first argument. that is probably the most natural.
Overview
Closes RQA-3567.
Test Plan and Hands on Testing
commandType: "home"intent: "fixit"command.Changelog
There are 4 substeps in an
aspiratecommand:blowOutand prepare for theaspirate.Prior work already lets us recover from overpressure and stall errors in steps 3 and 4. This does the same for steps 1 and 2.
Review requests
Anyone have a trick to triggering an overpressure during the prepare-for-aspirate substep? Is it actually very difficult to trigger, and maybe this is barking up the wrong tree to solve RQA-3567?
Risk assessment
Low.