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

doc(asgi): Another round of doc updates, this time centered around Request/Response #1667

Merged
merged 1 commit into from
Feb 15, 2020

Conversation

kgriffs
Copy link
Member

@kgriffs kgriffs commented Feb 6, 2020

Summary of Changes

Here's another round of ASGI-related doc improvements. This one centers mostly on things related to Request and Response.

Related Issues

#1358

@kgriffs
Copy link
Member Author

kgriffs commented Feb 6, 2020

I still need to double-check a few things but this is almost ready for review...

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #1667 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1667   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         3565      3568    +3     
  Branches       545       545           
=========================================
+ Hits          3565      3568    +3     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40f1ac7...0a291a9. Read the comment docs.

@@ -53,6 +108,7 @@ def __init__(self, receive, content_length=None):
self._pos = 0

def __aiter__(self):
"""Hello"""
Copy link
Member

Choose a reason for hiding this comment

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

I think you missed this

@kgriffs kgriffs force-pushed the docs-asgi-req-resp branch 2 times, most recently from 2321733 to 1f7f4c2 Compare February 8, 2020 01:10
attribute on the :py:class:`~.Request` object. Generally speaking, the
:py:meth:`~.Request.get_cookie_values` method should be used unless you need a
collection of all the cookies in the request.
:py:meth:`~.falcon.Request.get_cookie_values` method or the
Copy link
Member Author

Choose a reason for hiding this comment

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

I just fixed these up for now to resolve doc warnings, but I plan to circle back in all these places and sort out how to also point people to ASGI Request/Response docs.

@kgriffs kgriffs marked this pull request as ready for review February 8, 2020 01:18
@kgriffs kgriffs requested review from vytas7 and jmvrbanac February 8, 2020 01:18
vytas7
vytas7 previously approved these changes Feb 14, 2020
Copy link
Member

@vytas7 vytas7 left a comment

Choose a reason for hiding this comment

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

👍
I've just had a minor suggestion to improve SSEvent, not directly related to this PR (that can be addressed elsewhere).

@@ -5,6 +5,60 @@


class SSEvent:
Copy link
Member

Choose a reason for hiding this comment

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

A finding not directly related to the proposed changeset, but there seems to be some asymmetry between parameter validation in the constructor and the serialize()-ation function.

Passing a parameter that does not reduce to truth in a boolean test can go unnoticed in the up front check, e.g.:

>>> from falcon.asgi.structures import SSEvent
>>> event = SSEvent(text=0.0)
>>> event.serialize()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/vytas/progs/falcon/falcon/asgi/structures.py", line 85, in serialize
    block += 'data: ' + self.text + '\n'
TypeError: can only concatenate str (not "float") to str

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch. I'll address this in a followup PR.

@kgriffs kgriffs merged commit eea47a7 into falconry:master Feb 15, 2020
@kgriffs kgriffs deleted the docs-asgi-req-resp branch February 15, 2020 21:15
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.

3 participants