-
Notifications
You must be signed in to change notification settings - Fork 23
feat: use ValueError instead of TypeError for failed Operator.operation
#129
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
=======================================
Coverage 67.05% 67.05%
=======================================
Files 25 25
Lines 3160 3160
=======================================
Hits 2119 2119
Misses 1041 1041
🚀 New features to boost your workflow:
|
|
@sbillinge, it's ready for review. |
sbillinge
left a comment
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.
please see comments
| self._value = self.operation(*vals) | ||
| except Exception as e: | ||
| raise ValueError( | ||
| "Error evaluating operator '%s' with arguments %s : %s" |
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.
could we have this as an f-string for greater readability?
| self._value = self.operation(*vals) | ||
| try: | ||
| self._value = self.operation(*vals) | ||
| except Exception as e: |
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.
I think this should be TypeError maybe? Turning any exception into a valueError may lead to confusion?
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.
Yes. After a second thought, I think maybe we should not modify it at all.
The test-case-bad uses np.add but only gives it one argument. We want to use ValueError instead of TypeError in this bad case, because the input type is correct. But we can't customize all possible Errors that a user might encounter in their practice. I think the best approach is to leave the original error message and let the user see where they are wrong directly.
So, I think it is better to raise whatever the external function raises. In this case, we can close #72 as not planned.
|
I read the original issue and it was that get_value returned somewhere inside |
Oh, I see. I thought we need fix both |
|
Closed as the issue will be addressed in another cleaner PR. |
What problem does this PR address?
Closes #72
Use
ValueErrorinstead ofTypeErrorfor incorrect calculations.What should the reviewer(s) do?
Please check the modifications.