Skip to content

Require event hints for remote protocol errors#202

Open
Mirochill wants to merge 3 commits into
python-hyper:mainfrom
Mirochill:fix-178-require-event-hint
Open

Require event hints for remote protocol errors#202
Mirochill wants to merge 3 commits into
python-hyper:mainfrom
Mirochill:fix-178-require-event-hint

Conversation

@Mirochill
Copy link
Copy Markdown
Contributor

Summary

  • make RemoteProtocolError.event_hint a required constructor argument
  • update the public documentation to match the guarantee already provided by all internal call sites

Why

Issue #178 notes that every wsproto call site already supplies an event_hint, while the public signature still allows callers to construct a RemoteProtocolError without one. Requiring the argument makes the API describe the behavior the implementation already depends on and gives consumers a stable hint to react to.

This is intentionally an API-breaking change, matching the maintainer note that the issue is a candidate for a future minor API-breaking release.

Validation

  • No local validation run, per this workspace's coordination-only policy.
  • Verified statically that all in-repo RemoteProtocolError(...) call sites already provide event_hint=.
  • Relying on the repository's remote CI for execution feedback.

@Mirochill Mirochill marked this pull request as ready for review May 18, 2026 17:35
@Kriechi
Copy link
Copy Markdown
Member

Kriechi commented May 18, 2026

Please add a changelog entry to document the API breaking change as minor. Also add the corresponding source-code comment annotation.

Comment thread src/wsproto/utilities.py Outdated
"""

# API-breaking change for the next minor release: event_hint is required.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was thinking about https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-versionchanged

example:

    .. versionchanged:: 2.3.0
       Changed the type of ``headers`` to :class:`HeaderTuple
       <hpack:hpack.HeaderTuple>`. This has no effect on current users.

@Mirochill
Copy link
Copy Markdown
Contributor Author

Thanks, this is addressed in the follow-up commits: b1dc8d5 adds the 1.4.0 changelog entry, and f40f1b3 replaces the source note with a .. versionchanged:: 1.4.0 directive on RemoteProtocolError.

Remote checks now show the published tox jobs and codecov/patch green; codecov/project is still red, which looks like project coverage rather than this patch. No local tests were run, per my coordination-workspace policy.

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