burr icon indicating copy to clipboard operation
burr copied to clipboard

typing: `Application.step()`

Open zilto opened this issue 1 year ago • 1 comments

Currently, Application.step() gives a bunch of squiggly red lines when trying to unpack the return value.

image

This is because .step() is annotated as follow, potentially returning None, which can't be unpacked

def step(self, inputs: Optional[Dict[str, Any]] = None) -> Optional[Tuple[Action, dict, State]]:
   # ...
   return self._step(...)
   
def _step(
   self, inputs: Optional[Dict[str, Any]], _run_hooks: bool = True
) -> Optional[Tuple[Action, dict, State]]:
   # ... 
   return next_action, result, new_state

(the return line of ._step() also has red squiggly lines)

Does ._step() actually ever return None ? If yes, would it make sense to have it return (None, None, None) instead?

Having two potentially return values (the tuple or None) can be annoying to deal with if they have different "shapes". I'm guessing that directly unpacking the return value is common.

zilto avatar Sep 06 '24 13:09 zilto

OK, so:

  • _step does return None
  • Not sure what to do -- I need to figure out why. Effectively if you call step and nothing happens, you should know.
  • So whether we put that in the tuple or move that out is a bit odd
  • The only place we're calling this is in .step -- the other ones have specific conditions for hitting the point where it's none.
  • Agreed in ergonomics, although this is where a bit of typescript-type foo helps: (act, retval, state) = app.step() or (None, None, None). OTOH that type of code is just generally bad, so maybe this should just be fixed.

elijahbenizzy avatar Sep 06 '24 17:09 elijahbenizzy