fermi-ad / acsys-python

Python module to access the Fermilab Control System
MIT License
8 stars 2 forks source link

Create a `Status` factory method that generates `Exception` subclasses for desired ACNET statuses #45

Closed beauremus closed 2 years ago

beauremus commented 2 years ago

When trying to catch ACNET_DISCONNECTED status I get an error.

TypeError: catching classes that do not inherit from BaseException is not allowed

Example:

try:
    async def my_app(con):
        async with acsys.dpm.DPMContext() as dpm:
            await dpm.add_entry(0, drf)

            async for _ in dpm.replies():
                print('Received reply.')

    return acsys.run_client(my_app)
except ACNET_DISCONNECTED:
    print('Disconnected from ACNET')
    return

Is it possible to simply replace Exception with BaseException?

julianbadillo commented 2 years ago

Do you want to replace Exception to BaseException in class Status(Exception): ? I think that's not the issue, I think the problem is that ACNET_DISCONNECTED is an object, not a class.

julianbadillo commented 2 years ago

Maybe you want to do this instead?:

except Status as st:
   if st == ACNET_DISCONECTED:
      print('Disconnected from ACNET')
julianbadillo commented 2 years ago

From https://docs.python.org/3/library/exceptions.html#BaseException

exception BaseException The base class for all built-in exceptions. It is not meant to be directly inherited by user-defined classes (for that, use Exception).

beauremus commented 2 years ago

Let's consider using @staticmethod on Status as a way to create a factory that generates an Exception subclass for the desired ACNET_XXX statuses. This allows developers to catch the ACNET_XXX exceptions since the generated exceptions will inherit directly from Exception.

This was a discussion from Coding Club with @rneswold @julianbadillo and @beauremus.

rneswold commented 2 years ago

I think we were thinking along these lines:

class Status(Exception):
    # Defined the same as it is now

    # Add factory
    @staticmethod
    def create(val):
        if val == -47:
            return ACNET_DISCONNECT()
        elif val == -48:
            return ACNET_REQTMO()
        else:
            raise ValueError

# Define specialized subclasses

class ACNET_DISCONNECT(Status):
    def __init__():
        super.__init__(-47)

class ACNET_REQTMO(Status):
    def __init__():
        super.__init__(-48)

Something along those lines. So we can catch Status exceptions, or just catch ACNET_DISCONNECT exceptions.

rneswold commented 2 years ago

Via offline discussions, @beauremus and I also considered using Python's Enum framework to define the Status values. Enumerations would clean-up the code but won't, however, solve Beau's request to use status codes in except clauses.

julianbadillo commented 2 years ago

Agree with @rneswold , Enum won't be much different than creating constants with the error codes. I can work on this, be aware that the usage of ACNET_XX would change, hence breaking compatibility. I could leave the ACNET_XX constants (mark them as obsolete) and create sub-clases like:

class AcnetDisconnectException:
...

I'll start working on that, so you folks review my PR?

julianbadillo commented 2 years ago

Resolved on #46