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

Add fmt::Display implementation for MessageClass #16

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

thomaseizinger
Copy link
Contributor

No description provided.

@sile
Copy link
Owner

sile commented Sep 18, 2023

I don't have a strong negative opinion about this PR, but could you clarify the motivation behind it?
(As STUN is a binary protocol and (maybe) there is no official textual representation, I'm unsure if it is reasonable to define a textual representation in this crate.)

@thomaseizinger
Copy link
Contributor Author

I don't have a strong negative opinion about this PR, but could you clarify the motivation behind it? (As STUN is a binary protocol and (maybe) there is no official textual representation, I'm unsure if it is reasonable to define a textual representation in this crate.)

I am using the textual representation for metrics that I am recording about in and out-going messages and figured it would be handy to have them upstream!

@sile
Copy link
Owner

sile commented Sep 18, 2023

I see. Thank you for explaining!

Copy link
Owner

@sile sile left a comment

Choose a reason for hiding this comment

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

LGTM

@sile sile merged commit 814838b into sile:master Sep 18, 2023
11 checks passed
@thomaseizinger thomaseizinger deleted the feat/message-class-display branch September 18, 2023 23:18
github-merge-queue bot pushed a commit to firezone/firezone that referenced this pull request Sep 21, 2023
I've opened several PRs upstream for code that was missing in
`stun-codec` for our purposes. Those have been accepted and released, so
we can bump to that version now and remove that code.

Related: sile/stun_codec#14.
Related: sile/stun_codec#15.
Related: sile/stun_codec#16.
Related: sile/stun_codec#17.

A big thanks to @sile for the crate and being responsive maintainer
:partying_face:
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