-
Notifications
You must be signed in to change notification settings - Fork 22
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
All exceptions are WebSocketError. More specific exceptions possible? #28
Comments
I don't even know if Should it perhaps just be called |
Basically any exception from the websocket is a close. Would you handle WebSocketProtocolMismatchError, WebSocketHandshakeError differently? I think close should be an exception as the expected path for web socket to remain open forever. |
While I see what you mean, I also think that a specific error as possible should be raised. I checked the test cases and a bunch of them except WebSocketError and assume that the thing that went wrong is in fact what the test is for, but with that mentality we might as well just raise Exception since "it wouldn't be handled differently anyway" xP The test for protocol doesn't even except WebSocketError just 'except:' and then compares the error message. This seems unacceptable? a If the new exceptions are all of 'object of WebSocketError', old try-excepts would still work like they already do and the tests could become more specific. It wouldn't cost anything but would allow for cleaner code. Some code duplication could go away as well since the WebSocketClosedError is raised in quite a few places. Perhaps a simple 'newWebSocketClosedError' with the correct message should be made? |
I also beleive that this test (test_protocol_arr) is supposed to find a |
I would not mind WebSocketError hierarchy with WebsocketClosed, WebSocketProtocolMismatchError etc... I don't need it though, so I will not make this myself, but PRs welcome. My fear is that we add this complexity but its not used by anyone in real production system and just creates a bigger bug surface. |
Well, I believe that having more specific errors does the opposite of creating a bigger bug surface since it allows for better tests and debugging. Users can still just except WebSocketError if they don't care and catch them all. I'll make a PR since I do want this for myself anyway :) Should the README be updated as well to contain the new Errors? Should the chatserv er example still use |
Please update the examples. Don't worry about the README, we stopped using mddoc and use nimdocs now. I can change that part after your PR. |
I updated the read me to our new style. Feel free to mess with it. |
Added more exceptions that are all of type 'object of WebSocketError' so that it doens't break compatability with old try-except statements. Also updated some of the tests to except these more specific errors rather than the broad WebSocketError. Didn't change all though since I was unsure about some of them such as test_protocol_arr. closes treeform#28
Hi!
I see that all exceptions being raised are just
WebSocketError
. It is hard to only except "Closed" exceptions in one way and other exceptions in their own ways.Could we perhaps have something like
WebSocketClosedError
,WebSocketProtocolMismatchError
,WebSocketHandshakeError
and so on which could all beobject of WebSocketError
. This would ensure that scripts that already catchWebSocketError
would still work (right?).I might take a look tonight and see if I can submit a PR
The text was updated successfully, but these errors were encountered: