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 unused handle_no_connect on wsproto #1759

Merged
merged 1 commit into from Nov 17, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Nov 10, 2022

Neither RejectConnection nor RejectData have the reason field that handle_no_connect assumes both have, for that, I'm pretty confident this is never reached.

What those lines try to solve, it's already handled on data_receive.

@Kludex Kludex force-pushed the wsproto/remove-unused-handle-no-connect branch from 5405f61 to 69f979d Compare November 10, 2022 19:52
@Kludex Kludex requested a review from a team November 10, 2022 19:59
@Kludex Kludex added this to the Version 0.20.0 milestone Nov 10, 2022
This was referenced Nov 10, 2022
Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Agreed. This looks good IMO.

Do we have a logic for RejectData event in data_received?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 11, 2022

I don't understand the question.

The RemoteProtocolError is raised on data_received.

@iudeen
Copy link
Contributor

iudeen commented Nov 11, 2022

What I meant to ask was if we need to handle RejectData explicitly. If RemoteProtocolError is good enough, then okay.

@tomchristie
Copy link
Member

If RemoteProtocolError is good enough, then okay.

@iudeen - If you're happy with this, then you're welcome to review & accept it.

iudeen
iudeen approved these changes Nov 17, 2022
Copy link
Contributor

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Looks good! 😄

@Kludex
Copy link
Sponsor Member Author

Kludex commented Nov 17, 2022

Thanks! 🙌

@Kludex Kludex merged commit 53d7d1e into master Nov 17, 2022
15 checks passed
@Kludex Kludex deleted the wsproto/remove-unused-handle-no-connect branch November 17, 2022 12:40
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

3 participants