Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools.calculation: ExpressionEvaluator.Error should not be defined inside another class #2508

Open
dgw opened this issue Sep 21, 2023 · 0 comments
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Low Priority Tweak
Milestone

Comments

@dgw
Copy link
Member

dgw commented Sep 21, 2023

Because the Error raised by ExpressionEvaluator is defined here, inside the ExpressionEvaluator class itself:

class Error(Exception):
"""Internal exception type for :class:`ExpressionEvaluator`\\s."""
pass

Downstream consumers of the submodule's main (relevant) utility function can't just cleanly import the error type. They have to access it either as an attribute of one of the classes (ExpressionEvaluator or its child EquationEvaluator), or as an attribute of the "function" eval_equation() (which is actually an instance of EquationEvaluator, but 🤫).

This is more than a little weird as Python exception types go. Plus, if someone just lets Sopel's built-in error handler catch the exception, you get "Unexpected Error" in the output, because the class name is just "Error". Not very descriptive, that.

Proposal

Let's move the ExpressionEvaluator.Error type out into its own standalone class. In fact, it should probably become a full error hierarchy to cover the range of different error cases available. (WIP tests for the tools.calculation module currently have to substring-match the exception message to make sure the "right" Error is being raised.)

I've toyed with ideas for keeping the ExpressionEvaluator.Error name around for a while in case anyone else is using it (like I've just proposed doing in #2507), but am not dead set on doing so. The new standalone error type (hierarchy root, tentatively CalculationError) could start its life as a subclass of the old one, and then switch to subclassing Exception directly in the next major version. There won't be an effective hook point to show any deprecation warnings, though, so anyone using code that relies on the old (current) name will have to read release notes. 🤷‍♂️

@dgw dgw added Low Priority Tweak Breaking Change Stuff that probably should be mentioned in a migration guide labels Sep 21, 2023
@dgw dgw added this to the 8.1.0 milestone Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Stuff that probably should be mentioned in a migration guide Low Priority Tweak
Projects
None yet
Development

No branches or pull requests

1 participant