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

Remove conditional on RemoteProtocolError.event_hint #1486

Merged
merged 3 commits into from Oct 31, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented May 10, 2022

I'm not sure if this is the right code, but events.CloseConnection receives code as a mandatory field, so without it, you'll have an exception on missing parameters.

List of codes: https://www.iana.org/assignments/websocket/websocket.xhtml
RFC: https://www.rfc-editor.org/rfc/rfc6455.html

I've noticed this issue while annotating wsproto_impl.

EDIT: Quite funny... wsproto doesn't raise a RemoteProtocolError without an event_hint. Check their source code: https://github.com/python-hyper/wsproto/.

@Kludex Kludex changed the title Add code on events.CloseConnection for wsproto Add code on events.CloseConnection for wsproto May 10, 2022
@Kludex Kludex requested review from euri10, tomchristie and a team May 10, 2022 05:05
@Kludex Kludex self-assigned this May 10, 2022
@Kludex Kludex added this to the Version 0.18.0 milestone May 10, 2022
@tomchristie
Copy link
Member

Well spotted.

How about we use 1002 "Protocol Error"? That seems like it'd fit quite well here.

@Kludex
Copy link
Sponsor Member Author

Kludex commented May 13, 2022

The handle_no_connect() method ignores the code from the event anyway. I guess there needs to be some change there.

@Kludex Kludex marked this pull request as draft May 15, 2022 17:16
@Kludex Kludex modified the milestones: Version 0.18.0, Version 0.19.0 Jul 2, 2022
@Kludex Kludex modified the milestones: Version 0.19.0, Version 0.20.0 Oct 22, 2022
@Kludex Kludex mentioned this pull request Oct 29, 2022
13 tasks
@Kludex Kludex changed the title Add code on events.CloseConnection for wsproto Remove conditional on RemoteProtocolError.event_hint Oct 29, 2022
@Kludex Kludex marked this pull request as ready for review October 29, 2022 20:07
@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 29, 2022

I've replaced the logic here. 😅

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Do we pause this until there's a resolution to python-hyper/wsproto#178, or are we okay with knowing that event_hint isn't ever None in this context, although it's annotated as Optional?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 31, 2022

I'm fine with this being merged. Thanks @tomchristie 🙏

As a note, wsproto_impl.py is the only file that is missing to add type annotation on uvicorn folder. I'll work on that this week.

@Kludex Kludex merged commit 697588d into master Oct 31, 2022
15 checks passed
@Kludex Kludex deleted the code-1007-on-wsproto branch October 31, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants