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

make CloseReason an Enum? #59

Open
belm0 opened this issue Oct 17, 2018 · 4 comments
Open

make CloseReason an Enum? #59

belm0 opened this issue Oct 17, 2018 · 4 comments
Labels
enhancement New feature or request
Milestone

Comments

@belm0
Copy link
Member

belm0 commented Oct 17, 2018

i.e. enum with code as value, and a reason attribute

I'd like to be able to compare close reason without resorting to string comparison

@mehaase
Copy link
Contributor

mehaase commented Oct 17, 2018

I'd like this as well. There's already an enum in wsproto that could be reused: https://github.com/python-hyper/wsproto/blob/master/wsproto/frame_protocol.py#L85

Caveat: there are valid close codes that do not have names, so there still needs to be a way to instantiate instances with just a code number and optional reason string.

@belm0
Copy link
Member Author

belm0 commented Oct 17, 2018

It sounds like a good reason for trio-websocket to maintain its own enum. (Plus wsproto enum doesn't have reason string.)

Is anything of wsproto exposed currently?

@mehaase
Copy link
Contributor

mehaase commented Oct 17, 2018

Nothing in wsproto is part of the public trio-websocket API—except for the imported modules that are set to be hidden when the library is re-organized.

@mehaase mehaase added the enhancement New feature or request label Oct 25, 2018
@mehaase mehaase added this to the 1.0 milestone Nov 17, 2018
@belm0
Copy link
Member Author

belm0 commented May 21, 2019

I looked at this again:

  • trio-websocket CloseReason has many-to-one mapping to names like RFC_RESERVED. It's not enum-like. I suggest CloseReason have helper methods or properties for testing "is_rfc_reserved", "is_private_reserved", "is_valid", etc.
  • trio-websocket CloseReason is actually tuple of reason code and arbitrary message (and secondarily, a library-defined string name which may be used for matching, but probably shouldn't-- which is why we want enums). In this regard it's quite different than wsproto CloseReason.

Proposed:

  • CloseReason keeps code and reason message, but drops name property
    • code property remains type int, since it can be any number
    • add helpers for "is_rfc_reserved" etc.
    • in __str__(), attempt to convert code to a new enum (below) before formatting
  • add new CloseCode IntEnum, essentially mirroring wsproto CloseReason

I'm not a big fan of IntEnum due to str() gotchas, but it seems suitable in this case since we want users to easily compare the int code property with named values.

We could also consider having code being type [int | CloseCode], where CloseCode is still IntEnum. But as a user I think I'd rather have a simple, single type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants