-
Notifications
You must be signed in to change notification settings - Fork 61
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: make check_date_operators work with dateutil < 2.8.1 #273
fix: make check_date_operators work with dateutil < 2.8.1 #273
Conversation
for more information, see https://pre-commit.ci
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.
Oh! That's a much nicer approach than just forcing the version. I think this looks good in principle.
The linter is definitely going to complain about the bare except
here. I think that should be fixable, I think it's okay if you disable linting on that line
How do you feel about pulling that try catch up to a module level call rather than buried in the method itself?
OK @sighphyre, if you approve this approach I'm going to fix issues with the linter, etc. |
Okay, that's actually an entirely fair comment, good point. Yeah, I'm happy to merge this if the lint passes! |
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!
@snosratiershad I see the commit! Looks good to me, are you okay for me to merge it? |
@sighphyre yes, looks ready for me. |
Description
For better discussion about #266
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: