Skip to content
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 auth_secret identify option on python3 #217

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jul 20, 2018

The root issue is that on python3 there are conflicting type assertions for auth_secret:

class AsyncConn(...):
    def __init__(...):
         assert isinstance(auth_secret, string_types + (None.__class__,))
...
    def _on_response_continue(...):
        self.send(protocol.auth(self.auth_secret))

# in protocol.py ...

def auth(data):
    return _command(AUTH, data)

def _command(cmd, body, *params):
    assert isinstance(body, bytes_types), 'body must be a bytestring'

in #202, @ericb proposed just changing the initial type assertion to allow bytes_types instead of string_types. I'm fine with that, but I think it might be more "natural" to allow a string for auth_secret as well, so I've come up with my own variation using to_bytes.

(pushing just the test first to demonstrate failures, then the fix ...)

@ploxiln
Copy link
Member Author

ploxiln commented Jul 20, 2018

test isn't working at all on travis, dunno why, hang on a sec ...

@ploxiln
Copy link
Member Author

ploxiln commented Jul 20, 2018

Ah - test doesn't pass on python2.7 where I would expect it to, because of from __future__ import unicode_literals, and the failure is impossible to understand because the exception is hidden in a different context and not captured, annoyingly enough. Anyway the fix should fix it.

@ploxiln
Copy link
Member Author

ploxiln commented Jul 20, 2018

cc @jehiah

@ploxiln ploxiln merged commit 703ad18 into nsqio:master Jul 20, 2018
@ploxiln ploxiln deleted the auth_secret_py3 branch February 3, 2019 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants