Repository navigation
Conversation
0128379 to
ce1e1d3
Compare
ce1e1d3 to
ba9e901
Compare
Codecov Report
@@ Coverage Diff @@
## master #779 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 29
Lines 4388 4381 -7
Branches 406 404 -2
=========================================
- Hits 4388 4381 -7
Continue to review full report at Codecov.
|
| return copy | ||
|
|
||
| def get_safe(self, header: str) -> str: | ||
| from .exceptions import InvalidHeader |
There was a problem hiding this comment.
I guess you've had to do this to avoid an import cycle. This is a very common issue since I implemented static type checking with mypy.
The websockets.exceptions module is imported at the bottom at websockets.http, so you remove this import use websockets.exceptions.InvalidHeader.
It's ugly but I'd rather have a module-level import at the bottom of a module than scattered imports inside functions.
But if you're moving the code to websockets.handshake, the problem goes away.
| copy._list = self._list.copy() | ||
| return copy | ||
|
|
||
| def get_safe(self, header: str) -> str: |
There was a problem hiding this comment.
The only difference between get_safe and __getitem__ is that get_safe translates KeyError or MultipleValuesError into InvalidHeader. Having two methods that do the same thing in the same class is weird.
websockets.http.Headers is intended as a low-level utility that only knows about HTTP. This is why it uses websockets.http.MultipleValuesError instead of one of the websockets-specific exceptions in websockets.exceptions.
To factor out this behavior, I'd write a utility function in websockets.handshake. The job of this function would be to raise an exception from websockets.exceptions and to provide a good error message when the WebSocket protocol specifies that a given header must be provided exactly once, but it's there zero or multiple times.
You can call it get_unique_header for example.
|
I'm going to close this for now — we lost momentum — but I'm open to a PR implementing https://github.com/aaugustin/websockets/pull/779/files#r439800454 |
While doing the
raise fromfixes, I noticed that this logic can be refactored. Let's see whether the tests pass, and whether you find this code more elegant than the existing code :)