feat: use ValueError instead of TypeError for failed Operator.operation#129
feat: use ValueError instead of TypeError for failed Operator.operation#129ycexiao wants to merge 1 commit intodiffpy:mainfrom
ValueError instead of TypeError for failed Operator.operation#129Conversation
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. |
| 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.
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.
I think this should be TypeError maybe? Turning any exception into a valueError may lead to confusion?
There was a problem hiding this comment.
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.