ansys / ansys/pyfluent

Exception improvements

Open
#4,543 2 comments 0 reactions 0 assignees View on GitHub
design refactoring
Dominant language
Python
Stars
497
Forks
77
Avg merge
22h 37m
Merged PRs (30d)
45

Description

A few things I'm not a huge fan of currently with errors

1. There doesn't seem to be a class hierarchy for exceptions raised. Most libraries tend to have a "`PyFluentError`" that all custom exceptions inherit from in case you want to catch all the errors from the library.

2. Auditing [the code](https://github.com/search?q=repo%3Aansys%2Fpyfluent+%2Fclass+%5Cw%2B%5C%28%5Cw%2BError%2F&type=code) there seem to be a few exceptions that are entirely redundant and can be replaced with their std lib counter parts. e.g. Image
In this case this should just be a `ValueError`. Same for `UnexpectedKeywordArgument` that should just be a `TypeError`

3. There are also a lot of custom `TypeErrors` which isn't something you typically see. I would air on the side of just raising the exception directly for 2 reasons, it's shorter to read along with avoiding pollution the namespace with a bunch of errors which hurts IDE users who have to look through more options to find what they are looking for. I don't think anyone in normal code is catching a `WaitTypeError` so it's not losing functionality.

> Some developers may argue that, in this example, you can use the built-in `ValueError` instead of defining your own custom exception, and they might be right. In general, you’ll define custom exceptions when you want to point out project-specific errors or exceptional situations. - https://realpython.com/python-raise-exception/#choosing-the-exception-to-raise-built-in-vs-custom

I don't think the cases in this library really justify another error here

If you want to keep the consistent error messages maybe have all the error messages as strings in exceptions.py and just import those in the file that raises them and then in tests if that's something that needs doing.

I am heavily in favour of getting rid of most of these all together as they should be caught by type checkers (though unsure how this works with pyconsole)

4. The `InvalidSolutionVariableNameError` error could be slightly confusing due to its suffix, maybe there's a better name, `SolutionVariableNotFoundError`?

5. Conditional usage of `Exception.add_note()` will help to improve the already good current situation with guiding users towards the correct attribute name using the mechanism for enriching exceptions on versions greater than or equal to 3.11

Contributor guide

Open the contributing guide

Research direction

Start by auditing the custom exceptions referenced in exceptions.py and the linked code search, then review the existing tests for their usage. Define the exception hierarchy, decide which custom errors can become built-ins, assess the proposed rename, and check conditional Exception.add_note() support for Python 3.11+. Done means the exception API and related tests consistently reflect the agreed decisions.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend-api-design
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.