-
Notifications
You must be signed in to change notification settings - Fork 17
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 deprecation warnings. #53
Conversation
Fixed the ones which have shown up when running the tests in the way the warning suggested it.
@@ -111,7 +114,7 @@ def _doDelUsers(self, names): | |||
def identify(self, auth): | |||
if auth and auth.lower().startswith('basic '): | |||
try: | |||
name, password = decodestring(auth.split(' ')[-1].encode()).decode() \ | |||
name, password = decodebytes(auth.split(' ')[-1].encode()).decode() \ |
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.
We just compared auth
against native str above: are you confident it is really bytes?
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.
According to the documentation (https://docs.python.org/3/library/base64.html#base64.decodestring) base64.decodestring
is a deprecated alias of decodebytes
. I think at least my change does not make the code worse.
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.
OK, but if the Python3 stdlib is expecting bytes, we shouldn't be passing in auth
as text, nor comparing / splitting it with str
constants.
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've opened #56 to track this issue.
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.
@tseaver Thank you.
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.
Fixed in ad391da
Green Travis CI depends on merging #55 into this branch. |
Fixed the ones which have shown up when running the tests in the way the warning suggested it.