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

replace system errors #529

Merged
merged 7 commits into from
Mar 16, 2023
Merged

replace system errors #529

merged 7 commits into from
Mar 16, 2023

Conversation

homosapien-lcy
Copy link
Contributor

@homosapien-lcy homosapien-lcy commented Mar 14, 2023

System error is widely used in the envisage project. However, this type of error is not very descriptive. Thus in this PR we are attempting to replace these errors with more descriptive errors.

Closes #442

@homosapien-lcy homosapien-lcy changed the title replace system errors [WIP] replace system errors Mar 14, 2023
@homosapien-lcy homosapien-lcy changed the title [WIP] replace system errors replace system errors Mar 15, 2023
@homosapien-lcy
Copy link
Contributor Author

This is my first time work with various errors here, so I will put my thought process here:
There are 3 main category of systemerror used in this project:
1 related to plugin_id (such as line 158 in envisage/composite_plugin_manager.py), this is caused by plugin cannot be found for the specified plugin id and thus gives a None, I replaced it with value error (Raised when an operation or function receives an argument that has the right type but an inappropriate value)
2 related to no compatible egg is found (such as line 156 in envisage/egg_basket_plugin_manager.py), this is caused by an error during runtime, so I use RuntimeError
3 related to unknown ETSConfig.toolkit (such as line 48 in examples/legacy/plugins/workbench/AcmeLab/acme/workbench/view/color_view.py), this is caused by toolkit not found in the attribute, which is during runtime, I would classified as RuntimeError

Reference:
List of errors: https://docs.python.org/3/library/exceptions.html

envisage/egg_utils.py Outdated Show resolved Hide resolved
envisage/service.py Outdated Show resolved Hide resolved
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I have a few suggestions for changes.

@@ -15,7 +15,7 @@

# Enthought library imports.
from traits.api import TraitType

from traits.trait_errors import TraitError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's import from traits.api. (Most of the ETS packages are designed so that you should only need to import from the api modules; that way, the internal details can be changed without risking breaking client code.)

@homosapien-lcy
Copy link
Contributor Author

OK, new comments are addressed

@mdickinson
Copy link
Member

OK, new comments are addressed

Thank you! Just one left - please could you fix the TraitError import?

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mdickinson mdickinson merged commit 886381e into main Mar 16, 2023
@mdickinson mdickinson deleted the system_error_fix branch March 16, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't raise SystemError
2 participants