Add ExtensionFrame object #75
Conversation
| | ||
| @staticmethod | ||
| def parse_frame_header(header): | ||
| def parse_frame_header(header, strict=False): |
There was a problem hiding this comment.
Please add a docstring comment to explain the new parameter.
hyperframe/frame.py Outdated
| stream_id = fields[4] | ||
| | ||
| if type not in FRAMES: | ||
| if not strict: |
There was a problem hiding this comment.
This is purely preference, but I’d have done this as:
if strict: raise UnknownFrameError return ExtensionFramebecause we expect strict=True to be the unusual path – or at least, it seems so, given we default to False.
(I don‘t care if you change this or not, just noting my feelings.)
hyperframe/frame.py Outdated
| Although certain byte prefixes are ordained by specification to have | ||
| certain contextual meanings, frames with other prefixes are not prohibited, | ||
| and may be used to communicate arbitrary meaning between HTTP 2 peers. |
There was a problem hiding this comment.
Nitpick: it’s “HTTP/2”, not “HTTP 2”.
(No, I don’t know why either.)
test/test_frames.py Outdated
| @@ -1,4 +1,6 @@ | |||
| # -*- coding: utf-8 -*- | |||
| from hyperframe.flags import Flag | |||
| | |||
There was a problem hiding this comment.
Nitpick: any reason for the empty line?
There was a problem hiding this comment.
Nope. Completely stylistic.
| f, l = Frame.parse_frame_header( | ||
| b'\x00\x00\x59\xFF\x00\x00\x00\x00\x01' | ||
| ) | ||
| assert f.type == 0xFF |
There was a problem hiding this comment.
I think we could additionally have
assert isinstance(f, ExtensionFrame)
| @alexwlchan, I believe I've addressed your comments. |
| Takes a 9-byte frame header and returns a tuple of the appropriate | ||
| Frame object and the length that needs to be read from the socket. | ||
| This populates the flags field, and determines how long the body is. |
There was a problem hiding this comment.
Can you add a versionchanged stanza in this docstring to indicate what version we changed this and what the change was? In this case, the version we changed it would be 5.0.0.
| flag_byte=flags, | ||
| stream_id=stream_id, | ||
| ) | ||
| return (frame, length) |
There was a problem hiding this comment.
So, for the sake of clarity, let's actually do this:
try: frame = FRAMES[type](stream_id) except KeyError: if strict: raise UnknownFrameError(type, length) frame = ExtensionFrame(type=type, stream_id=stream_id)Then, the flag byte can end up in place using parse_flags, and we can reduce the amount of return points.
There was a problem hiding this comment.
So, I don't think we can actually use parse_flags here. parse_flags depends on us having a definition of which flags are available. Since the meaning of the flags is defined on a per-frame basis, the base ExtensionFrame doesn't have the ability to do that.
The assign_flag_mapping method lets us do that after the fact - an upstream consumer, which is interpreting the frame, can insert a mapping between bit location and flag name, resulting in the frame getting a flags member.
There was a problem hiding this comment.
So I am not sure that assign_flag_mapping is a good idea tbh. I think that realistically anyone who wants to interpret the body of a new frame has to handle basically everything except the parsing of the frame header. So I'd be happy to just have us save the flag_byte and call it a day.
| Thus, hyperframe, rather than raising an exception when such a frame is | ||
| encountered, wraps it in a generic frame to be properly acted upon by | ||
| upstream consumers which might have additional context on how to use it. | ||
| """ |
There was a problem hiding this comment.
Can you add a versionadded stanza to this docstring?
| """ | ||
| self.defined_flags = flags | ||
| self.flags = Flags(self.defined_flags) | ||
| return self.parse_flags(self.flag_byte) |
There was a problem hiding this comment.
I don't think we need this method.
There was a problem hiding this comment.
See comment above.
| ) | ||
| assert f.body == b'hello world!' | ||
| assert f.body_len == 12 | ||
| |
There was a problem hiding this comment.
Can at least one of these tests validate that the stream id is correct?
| @Lukasa, addressed a couple of your comments and responded on a couple other. |
| flag_byte=flags, | ||
| stream_id=stream_id, | ||
| ) | ||
| return (frame, length) |
There was a problem hiding this comment.
So I am not sure that assign_flag_mapping is a good idea tbh. I think that realistically anyone who wants to interpret the body of a new frame has to handle basically everything except the parsing of the frame header. So I'd be happy to just have us save the flag_byte and call it a day.
3 participants
This PR adds the ExtensionFrame object, which hyperframe can use to denote a frame whose identifier code isn't known to the package. This can be used in upstream consumers which do know the semantic meaning of that particular identifier code.