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

Removing trailing space after a command if there are no arguments. #311

Merged
merged 4 commits into from
Apr 14, 2021

Conversation

jerrymiu
Copy link
Contributor

According to the Memcached Protocol, the no argument form of the stats command should have no trailing spaces. Currently, b'stats \r\n' instead of b'stats\r\n' is being sent to the socket which leads to a malformed response.

According to the Memcached Protocol, https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L1151 the no argument form of the stats command should have no trailing spaces. Currently, b'stats \r\n' instead of b'stats\r\n' is being sent to the socket which leads to a malformed response.
pymemcache/client/base.py Outdated Show resolved Hide resolved
@jparise
Copy link
Collaborator

jparise commented Apr 14, 2021

It looks like you'll also need to update the stats tests with the new (correct) behavior:

pymemcache/test/test_client.py:925: in test_stats_conversions
    assert client.sock.send_bufs == [
E   AssertionError: assert [b'stats\r\n'] == [b'stats \r\n']
E     At index 0 diff: b'stats\r\n' != b'stats \r\n'
E     Full diff:
E     - [b'stats \r\n']
E     ?         -
E     + [b'stats\r\n']

@jerrymiu
Copy link
Contributor Author

Noted, thanks I've updated the test cases and I got the automated checks to pass.

@jparise
Copy link
Collaborator

jparise commented Apr 14, 2021

Noted, thanks I've updated the test cases and I got the automated checks to pass.

Good stuff. Thanks for your contribution!

@jparise jparise merged commit f232bdd into pinterest:master Apr 14, 2021
@jerrymiu
Copy link
Contributor Author

You're welcome!

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