-
-
Notifications
You must be signed in to change notification settings - Fork 682
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
Added Error Checking For Empty Granularity List #1016
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1016 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 2221 2223 +2
Branches 349 350 +1
=========================================
+ Hits 2221 2223 +2
Continue to review full report at Codecov.
|
arrow/arrow.py
Outdated
if not granularity: | ||
raise ValueError( | ||
"Empty granularity list provided. " | ||
"Please select between 'second', 'minute', 'hour', 'day', 'week', 'month' or 'year'." |
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.
The second sentence should be reworded since this is a list. It's currently phrased as if only one choice can be made.
Co-authored-by: Chris <[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.
Small comment
arrow/arrow.py
Outdated
if not granularity: | ||
raise ValueError( | ||
"Empty granularity list provided. " | ||
"Please select one or more from ['second', 'minute', 'hour', 'day', 'week', 'month', 'quarter', 'year']." |
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.
It's probably better to write this as a plain english list rather than as a list. We should probably just join the member variable like we do here:
Lines 1026 to 1029 in 2ade710
supported_attr = ", ".join(self._ATTRS_PLURAL + additional_attrs) | |
raise ValueError( | |
f"Invalid shift time frame. Please select one of the following: {supported_attr}." | |
) |
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 concur with this. Makes it easier to digest as an error.
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.
Minor change to make it more dynamic, so we can get rid of the hardcoded strings.
Do we want to get this in |
@jadchaar worth adding this into the |
Sure let me get it up to date with master and add it to 1.2.1. |
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
Pull Request Checklist
Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:
tox
ormake test
to find out!).tox -e lint
ormake lint
to find out!).master
branch.If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!
Description of Changes
Closes #1015. Added a basic error check to make sure empty granularity list raises a Value Error.