Skip to content

Conversation

@mkrasnitski
Copy link
Collaborator

Follow-up to #3369 - using StdResult isn't necessary because we can already coerce tungstenite::Error into serenity's Error. Worth noting that this incurs a Box call for the error type, but this isn't a hot path by any means.

@github-actions github-actions bot added the gateway Related to the `gateway` module. label Jun 30, 2025
Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

This was a purposeful optimisation to avoid returning the large enum of serenity's Error and instead just return the smaller tungstenite error. Since these are internal methods, it doesn't matter to be hiding the error type, so I cannot see the benefit here.

If you have a benefit to reverting this, let me know, otherwise I would like to see this PR closed.

@mkrasnitski
Copy link
Collaborator Author

Turns out serenity::Error is pretty small, 72 bytes. And, tokio_tungstenite::tungstenite::Error is 136 bytes large. So, unless we box it, we're not getting any benefit. Plus, mixing result types like this is odd imo.

Copy link
Member

@GnomedDev GnomedDev left a comment

Choose a reason for hiding this comment

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

Fair enough.

@GnomedDev GnomedDev merged commit 1d7b9e7 into serenity-rs:next Jul 25, 2025
24 checks passed
@mkrasnitski mkrasnitski deleted the ws-result branch July 25, 2025 15:21
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Jul 28, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gateway Related to the `gateway` module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants