[FeatureRequest] PyCyphal FileClient remote functions should raise

[hannesweisbach]

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.

PS: Sorry for the spam. I submitted this issue from the wrong user account :(

[pavel-kirienko]

This is a valid point, and there is merit in unifying the error handling approach. The reason it is built the way it is is that there was an attempt to clearly separate the scope of responsibility of the FileClient -- the idea was that as long as it managed to complete the network transaction successfully, its job is done, the rest is up to the application to handle. I see now, however, that this is not a good design for an application-layer module.

Instead of adding a new method parameter, I recommend defining a new class; let's say we call it FileClient2, which implements the new error handling approach, and we mark the old class deprecated.