-
Notifications
You must be signed in to change notification settings - Fork 43
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
Custom Higher Level Exceptions #2176
Conversation
hpohekar
commented
Oct 26, 2023
•
edited
Loading
edited
- Added higher level exceptions.
- Corrected existing exceptions.
- Updated existing tests.
- Added new tests.
- Update new exception names
- Update error message
- Update docstring of exception class
- Add error message at the definition of exception class
- Update tests - just raise and catch exceptions.
- Correct grammar of existing error messages.
- Incorporate some changes of TaskObject updates. #2150.
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.
Looks good to me.
By the way, do you plan on adding more tests for the custom exceptions? I think that would be a good addition
Yes, we will add more tests. I haven't explored all test files yet. |
In yesterday's discussion you have mentioned perfect point of simultaneous implementation of suggestions from multiple reviewers makes the PR chaotic. Personally I try to avoid merging a PR with lot of commits therefore I end up in creating multiple PRs for same task. Actually, the first PR was in draft mode and it was work in progress. I hadn't completed all tasks required to get it ready for review, but multiple suggestions were started coming from reviewers which is perfectly fine. I remember that your first comment was about exact behavior that we wanted, but I did not continue my own design approach. The only issue was I kept myself too much open to address each and every suggestion by implementing it directly without completing my own design approach first. I agree that there was a regression initially but eventually it would have caught in testing. Testing files were not updated at that time. Customized exceptions is general in terms of naming of exceptions and error messages. I agree that each developer can think to refine exception name and message again and again to help user to get better understanding of error scenario based on our own experience and expertise. I have created task list based on your suggestions and I will implement it incrementally. Thank you. |
@seanpearsonuk This PR is ready. |