-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
This is my first time work with various errors here, so I will put my thought process here: Reference: |
There was a problem hiding this 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.
envisage/service.py
Outdated
@@ -15,7 +15,7 @@ | |||
|
|||
# Enthought library imports. | |||
from traits.api import TraitType | |||
|
|||
from traits.trait_errors import TraitError |
There was a problem hiding this comment.
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.)
OK, new comments are addressed |
Thank you! Just one left - please could you fix the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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