Skip to content

Conversation

@folospior
Copy link
Contributor

See rawhat/gramps#3.

In essence, in addition to the standard 1000 close codes, APIs can define their own. This PR brings things up to date with the gramps PR.

Note: PR is a draft because it currently has a git dependency on my fork. Will change once gramps PR is approved and pushed to Hex.

@folospior folospior force-pushed the main branch 3 times, most recently from 4c05cc9 to 15cf013 Compare August 9, 2025 22:47
@folospior folospior marked this pull request as ready for review August 9, 2025 22:47
@folospior
Copy link
Contributor Author

Here we go, took a lot of retries but I got it right (PC format and my git config wasn't quite right).

@rawhat Ready for review :)

Planning to add some features later on (more close reasons, on_close w/CloseReason [i actually depend on it lol])

@folospior
Copy link
Contributor Author

folospior commented Aug 21, 2025

By the way, I actually implemented an on_close function with close reasons (to be able to pattern match based on what we receive from the server), do you think I should merge that branch into this PR or make a separate one once this is done (or as a draft)

I'll make a new one once this is merged, please don't make a release until then (to avoid two major releases)

@folospior
Copy link
Contributor Author

@rawhat Cleaned up the mess :)
Will rebase folospior#1 when this gets merged

@folospior
Copy link
Contributor Author

Will rebase folospior#1 when this gets merged

At least I hope I can, I haven't dived into stacked PRs before 😅

}

/// This closes the WebSocket connection with a particular close reason.
pub fn close_with_reason(
Copy link
Owner

Choose a reason for hiding this comment

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

from an ergonomics perspective, do you think making the CloseReason non-opaque and just keeping the close and close_with_reason as the only public functions? i'm not really a user of this library, so curious about your perspective

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, I think you got a point. The only close reason that actually needs to be opaque is the custom one, so that could be in its own opaque type with its own function. I'll get around to it in a few. It could introduce some naming problems with sent and received close reasons, but it's not anything we can't figure out.

folospior and others added 5 commits September 7, 2025 16:57
@folospior folospior requested a review from rawhat September 7, 2025 15:10
@folospior
Copy link
Contributor Author

I also added some labels and bugfixes (preventing negative and invaid close codes), I hope these won't be an issue (I should've split commits 😞)

@rawhat rawhat merged commit d5837b3 into rawhat:main Sep 7, 2025
1 check passed
@rawhat
Copy link
Owner

rawhat commented Sep 7, 2025

Thank you!

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.

2 participants