-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Improve DateRange validation #627
Conversation
Codecov Report
@@ Coverage Diff @@
## master #627 +/- ##
==========================================
+ Coverage 82.81% 82.82% +0.01%
==========================================
Files 5 5
Lines 3037 3039 +2
==========================================
+ Hits 2515 2517 +2
Misses 522 522
Continue to review full report at Codecov.
|
@jlstevens ready for review 🙃 |
Looks good to me! |
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, thanks! I agree that it would be nice to make messages more consistent, but don't have a terribly strong opinion between the ways that type information is printed. I think on balance I'd prefer a simpler, more-English like version (e.g. type float
) over a syntax-y version (e.g. <class 'float'>
).
Agreed, consistency would be good but for now this is an improvement. |
Noticed that the
DateRange
parameter was not checking that the value provided is atuple
, which it should be given thatDateRange
inherits from Tuple.I've also changed the error message reported when one of the values is not of the correct type:
This is slightly different formatting compared to the messages reported by other Parameters, e.g. with a List:
Notice
of type float
vs.of type <class 'str'>
. If the latter is preferred, I could update the error messages of the other Parameters.