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
Drop support for legacy Python 2.7 #153
Conversation
First off, thank you so much for your contribution. I really appreciate it! I'm not an expert in python2 to python3 migrations, but I'll review it. It also looks like we should probably add an entry to the CHANGES.rst (that's what the bot is complaining about). Feel free to add that in. |
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.
All this looks very good and consistent to me. Please add a CHANGES.rst note that we're dropping python2 and that should make the bot green. Since this is a pretty big change, and I'm not a python expert or anything, does anyone else want to chime in on this?
@@ -18,14 +12,14 @@ | |||
__all__ = "STRING BINARY NUMBER DATETIME ROWID".split() | |||
|
|||
|
|||
class Error(StandardError): | |||
class Error(Exception): |
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.
beyond the scope of this PR, but I'm not sure what use of these base classes have, why not use the standard python Warning
and a sensible Exception class to subclass from?
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.
Changes look good.
I suspect there may be some other stuff that can be cleaned up (there is always), but all changes here looks good. |
Okay everything seems good once we get a changes.rst entry! |
CHANGES.rst updated! |
Awesome, thanks so much for your help! |
Fixes #120.