falconry / falcon

The no-magic web data plane API and microservices framework for Python developers, with a focus on reliability, correctness, and performance at scale.
https://falcon.readthedocs.io/en/stable/
Apache License 2.0
9.51k stars 937 forks source link

feat(websocket): add support for reason in WebSocket.close() #2056

Closed wendy5667 closed 1 month ago

wendy5667 commented 2 years ago

Summary of Changes

This PR adds reason to Websocket.close() as specified in version 2.3 of the HTTP & WebSocket ASGI Message Format. Closure due to HTTPErrors are rendered automatically from HTTP status code, while others are rendered by a default mapping, which could be changed by users.

Related Issues

Closes #2025

Pull Request Checklist

codecov[bot] commented 2 years ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (206bb38) to head (a53ba4c). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2056 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 63 63 Lines 7178 7218 +40 Branches 1260 1268 +8 ========================================= + Hits 7178 7218 +40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

CaselIT commented 2 years ago
  • I'm not sure if we can always send() a reason indiscriminately inside an ASGI WebSocket protocol message. Maybe it's safer to only include a reason iff we've verified we're dealing with ASGI HTTP+WebSocket protocol spec version 2.3+?

I agree, this seems the safest option

wendy5667 commented 2 years ago
  • We need to write a newsfragment for the new feature you've implemented.

Could you kindly share the style guide for newsfragments? I couldn't find it in the Contributor's Guide. Thank you!

CaselIT commented 2 months ago

sorry @wendy5667 we kinda forgot this PR. I'll try to bringing it over the finishing line.

vytas7 commented 1 month ago

Furthermore, unfortunately it seems that the code got affected by a botched master merge. I can try to fix these things.