Deriving all custom error from Exception and related

[germa89]

I'm not very convinced on this one...

The idea is to have all the MAPDL error classes deriving from one (MapdlException), so we can do something like:

try:
    mapdl = launch_mapdl()
except MapdlException:
    # Exception raised by PyMAPDL handleing this different
    handle_pymapdl_exception()
except Exception:
    # handles everything else

With the current implementation, you can do this too:

try:
    raise MapdlValueError
except MapdlValueError:
    # this is activated (ofc)
    handle_value_error()
except MapdlException:
    # this could be also activated
    handle_value_error()
except MapdlException:
    # this could be also activated
    handle_value_error()
except Exception:
    # this could be also activated
    handle_value_error()

I think it could add value, but it does generate maybe complicated inheritance relationships:

graph TD;
    Exception-->ValueError;
    Exception-->MapdlException;
    ValueError-->MapdlValueError;
    MapdlException--> MapdlValueError;

Loading

Because MapdlValueError inheritates from ValueError(which inheritates from Exception) and from MapdlException (which inheritates from Exception).
This creates a "diamond" which is not recommended diamond problem

Surely this classes are simple, and we are only overwritting __init__ method... if anything, we can always go back?

Related (but not much) #2653

Pinging @ansys/pyansys-core for technical feedback. Also pinging @koubaa @greschd

[ansys-reviewer-bot]

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

[germa89]

I think it could add value, but it does generate maybe complicated inheritance relationships:

Here it goes: https://github.com/ansys/pymapdl/actions/runs/7544725242/job/20538709809?pr=2671#step:3:58

GitHub

Deriving all custom error from Exception and related · ansys/pymapdl@746106d

Pythonic interface to MAPDL. Contribute to ansys/pymapdl development by creating an account on GitHub.

[greschd]

Typically I wouldn't worry about the diamond inheritance if all you're doing is inheriting from "empty" classes for the purposes of enabling an isinstance check.

However, I also found this: https://docs.python.org/3/library/exceptions.html#inheriting-from-built-in-exceptions

It’s recommended to only subclass one exception type at a time to avoid any possible conflicts between how the bases handle the args attribute, as well as due to possible memory layout incompatibilities.

I'm not entirely sure if this guidance applies here. As the MapdlException's base Exception is anyway already in the inheritance chain, I would tend to think it's not a problem.
But, you could also change it to be [1]

class MapdlExceptionMixin:
    """Mixin class to mark an exception type as originating from MAPDL"""

which would avoid inheriting from the built-in exceptions twice.

[1] remove the base Exception, and rename MapdlException to MapdlExceptionMixin to indicate it's not a standalone exception class.

Python documentation

Built-in Exceptions

In Python, all exceptions must be instances of a class that derives from BaseException. In a try statement with an except clause that mentions a particular class, that clause also handles any excep...

[greschd]

Here it goes: https://github.com/ansys/pymapdl/actions/runs/7544725242/job/20538709809?pr=2671#step:3:58

I think that might've worked if MapdlException was the right-hand-side in that inheritance.

GitHub

Deriving all custom error from Exception and related · ansys/pymapdl@746106d

Pythonic interface to MAPDL. Contribute to ansys/pymapdl development by creating an account on GitHub.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (183923c) 84.81% compared to head (520c19f) 82.86%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
- Coverage   84.81%   82.86%   -1.96%     
==========================================
  Files          47       47              
  Lines        9313     9319       +6     
==========================================
- Hits         7899     7722     -177     
- Misses       1414     1597     +183     

[germa89]

Here it goes: https://github.com/ansys/pymapdl/actions/runs/7544725242/job/20538709809?pr=2671#step:3:58

I think that might've worked if MapdlException was the right-hand-side in that inheritance.

this one was an easy one because DeprecationError inheritate also from MapdlException. So I just removed one here: ec36e9a

GitHub

Deriving all custom error from Exception and related · ansys/pymapdl@746106d

Pythonic interface to MAPDL. Contribute to ansys/pymapdl development by creating an account on GitHub.

[germa89]

Typically I wouldn't worry about the diamond inheritance if all you're doing is inheriting from "empty" classes for the purposes of enabling an isinstance check.

However, I also found this: https://docs.python.org/3/library/exceptions.html#inheriting-from-built-in-exceptions

It’s recommended to only subclass one exception type at a time to avoid any possible conflicts between how the bases handle the args attribute, as well as due to possible memory layout incompatibilities.

I'm not entirely sure if this guidance applies here. As the MapdlException's base Exception is anyway already in the inheritance chain, I would tend to think it's not a problem. But, you could also change it to be [1]

class MapdlExceptionMixin:
    """Mixin class to mark an exception type as originating from MAPDL"""

which would avoid inheriting from the built-in exceptions twice.

[1] remove the base Exception, and rename MapdlException to MapdlExceptionMixin to indicate it's not a standalone exception class.

Python documentation**Built-in Exceptions**In Python, all exceptions must be instances of a class that derives from BaseException. In a try statement with an except clause that mentions a particular class, that clause also handles any excep...

I'm also in the opinion on going ahead because as you mention the classes are not really complex and they are only for catching in try and isinstance.

Thank you a lot for your review @greschd !!

Python documentation

Built-in Exceptions

In Python, all exceptions must be instances of a class that derives from BaseException. In a try statement with an except clause that mentions a particular class, that clause also handles any excep...

[germa89]

Interesting read: http://python-history.blogspot.com/2010/06/method-resolution-order.html

Method Resolution Order

In languages that use multiple inheritance, the order in which base classes are searched when looking for a method is often called the Metho...

[clatapie]

LGTM!