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 #976: Add support for websocket send_json and receive_json #984

Merged
merged 8 commits into from
Jul 24, 2016

Conversation

vitaliemaldur
Copy link
Contributor

@vitaliemaldur vitaliemaldur commented Jul 23, 2016

What do these changes do?

Add support for websocket server to send json and for websocket client to send and receive json

Are there changes in behavior for the user?

User can use methods:
send_json and receive_json both on websocket server and client

Related issue number

#976

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.2%) to 97.803% when pulling 7fb73d0 on vitaliemaldur:master into 328bb36 on KeepSafe:master.

@@ -154,6 +154,13 @@ def send_bytes(self, data):
type(data))
self._writer.send(data, binary=True)

def send_json(self, data, *, dumps=json.dumps):
Copy link
Member

@asvetlov asvetlov Jul 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please call .send_str() instead of working on low level.

@asvetlov
Copy link
Member

Would you add tests also?

@vitaliemaldur
Copy link
Contributor Author

vitaliemaldur commented Jul 23, 2016

Yes I will

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.1%) to 97.864% when pulling 611a286 on vitaliemaldur:master into 328bb36 on KeepSafe:master.

@vitaliemaldur
Copy link
Contributor Author

@asvetlov what I can do to avoid

AttributeError: 'module' object has no attribute 'JSONDecodeError'
tests\test_web_websocket.py:161: AttributeError

I am using json.decoder.JSONDecodeError in a try/except.

Also, do you have some suggestions for the tests I wrote?

@@ -154,6 +154,13 @@ def send_bytes(self, data):
type(data))
self._writer.send(data, binary=True)

def send_json(self, data, *, dumps=json.dumps):
if self._writer is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks are not required: .send_str() performs them anyway.
There is no need to do the check twice.

@vitaliemaldur
Copy link
Contributor Author

vitaliemaldur commented Jul 23, 2016

@asvetlov I've noticed that in web_ws.py we have send_str, send_bytes, send_json methods and coresponding receive_str, receive_bytes, receive_json. But in websocket_client.py we have send_str, send_bytes, send_json methods and only receive_json. Is that on purpose? or should I add the rest of methods there?

@asvetlov
Copy link
Member

I think yes.
From my perspective we should support the same methods for both client and server for consistency.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.04%) to 97.945% when pulling 66c2c00 on vitaliemaldur:master into 328bb36 on KeepSafe:master.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.04%) to 97.945% when pulling 524669e on vitaliemaldur:master into 328bb36 on KeepSafe:master.

@coveralls
Copy link

coveralls commented Jul 23, 2016

Coverage Status

Coverage decreased (-0.008%) to 97.974% when pulling 8fd4aa9 on vitaliemaldur:master into 328bb36 on KeepSafe:master.

@vitaliemaldur
Copy link
Contributor Author

@asvetlov I have added all the methods discussed above, now we have:

  • send_str
  • send_bytes
  • send_json
  • receive_str
  • receive_bytes
  • receive_json

both on websocket_client.py and web_ws.py. I have also added documentation for them.

@asvetlov asvetlov merged commit 2488b85 into aio-libs:master Jul 24, 2016
@asvetlov
Copy link
Member

Thanks!

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants