-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix type errors not TypeError
#1388
Conversation
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.
Invalid Literal values should raise ValueErrors
Reverted some of the changes now @erlendvollset - if you want to take another look |
There are still a bunch of ValueErrors that have been changed to TypeErrors for invalid literal values. |
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.
You have found a lot of wrong exceptions, please re-open this PR 😄 (I commented on the three first, but there are several more!)
Following Håkons comments
@haakonvt Updated now - hope I have got it (more) right this time 🥲 |
|
||
Returns: | ||
int: The total number of datapoints | ||
""" | ||
if self.is_string: | ||
raise ValueError("String time series does not support count aggregate.") | ||
raise TypeError("String time series does not support count aggregate.") |
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 go with RuntimeError
instead
Co-authored-by: Håkon V. Treider <[email protected]>
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! 🚀
@jonasdmarkussen A few tests that needs to be updated after the changes:
|
TypeError
Codecov Report
@@ Coverage Diff @@
## master #1388 +/- ##
==========================================
- Coverage 90.70% 90.69% -0.02%
==========================================
Files 119 119
Lines 14524 14524
==========================================
- Hits 13174 13172 -2
- Misses 1350 1352 +2
|
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 👌👌
Description
Changed errortype from ValueError to TypeError on relevant places in 14 files.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.