OpenCyphal / pycyphal

Python implementation of the Cyphal protocol stack.
https://pycyphal.readthedocs.io/
MIT License
117 stars 105 forks source link

[FeatureRequest] PyCyphal FileClient remote functions should raise #326

Closed neosatuseracc closed 5 months ago

neosatuseracc commented 5 months ago

Hello,

is there a specific reason why FileClient remote operations do raise FileTimeoutError, but return an error code for everything else? I think it is not good API because of its split error handling and makes handling errors actually cumbersome, for example for read():

try:
  data = fileClient.read()
  if isinstance(data, int):
    ... # handle error
except FileTimeoutError:
   printf('Access timed out.')
finally:
  fileClient.close()

I would propose raising an Exception from which the value of uavcan.file.Error can be retrieved, if so desired:

try:
  data = fileClient.read()
except FileAccessError as e:
  printf(f'Error code {e.value}')
except FileTimeoutError:
   printf('Access timed out.')
finally:
  fileClient.close()

I don't think isinstance - or checking the type of a value in general - is a particularly good error handling technique, especially when there are other parts of the API that use exceptions for error signalling.

I realise that this change would be breaking the API, so I'm also imagining a parameter which a user can set to select between the API versions:

class Fileclient:
  ...
  async def read(path: str, offset: int = 0, size: int | None = None, raise_on_error: bool = False)→ int | bytes:
    if not raise_on_error:
      printf("Deprecation warning")
    ...

If there is interest on this I'd be volunteering to give it a shot in implementing.