Clean up linter failures #14
Conversation
Codecov Report
@@ Coverage Diff @@
## master #14 +/- ##
==========================================
+ Coverage 81.96% 82.09% +0.12%
==========================================
Files 5 5
Lines 693 698 +5
Branches 163 163
==========================================
+ Hits 568 573 +5
Misses 76 76
Partials 49 49
Continue to review full report at Codecov.
|
|
Is there a setting for |
|
I think we don't need it? I think the |
|
Ah that's probably the setting I was thinking about. :) Carry on! |
| elif payload_len == 127: | ||
| data = yield from self._consume_exactly(8) | ||
| (payload_len,) = struct.unpack("!Q", data) | ||
| if payload_len < 2 ** 16: |
There was a problem hiding this comment.
Could 2 ** 16 be converted to a constant?
There was a problem hiding this comment.
It certainly could be, yeah. Not sure how much value there is in that though. It's possible, in fact, that python pre-compiles that value.
There was a problem hiding this comment.
And in fact it does. Here's the output from dis.dis on this code, truncated to just cover line 157:
157 118 LOAD_FAST 2 (payload_len)
120 LOAD_CONST 14 (65536)
122 COMPARE_OP 0 (<)
124 POP_JUMP_IF_FALSE 134
As you can see, something (probably Python's peephole optimiser) spots this static mathematical operation and compiles it into a single constant. So I don't think we need to worry about the conversion. =)
There was a problem hiding this comment.
Oh not just from a speed perspective (although I didn't know Python had aggressive pre-compiling like that), also from a 'what-does-this-value-mean' perspective. I guess our audience is already knowledgeable about RFC 6455 so if we leave this value here it's fine with me. :)
There was a problem hiding this comment.
Yeah, so I get that. What isn't clear to me is whether this is clearer. I am not super familiar with RFC 6455, but this reads to me as being if payload_len < MAX_16_BIT_UINT. The exception on the next line basically acts as a code comment explaining what the check is. So, I don't know that the named constant would help much, if at all.
There was a problem hiding this comment.
I agree, gotta remember our audience with this package. 😛
There was a problem hiding this comment.
So this is in section 5.2 of RFC 6455. The payload length can be expressed in three different ways and it's required that you use the one most appropriate. I agree that we should be using names here for comprehensibility purposes but the standard doesn't give us any obvious names. The values in question are:
- 125 (Maximum normal (or short?) length)
- 126 (Flag value for two-byte extended length)
- 127 (Flag value for eight-byte extended length)
- 2^16 (Maximum two-byte extended length)
- 2^64 (Maximum eight-byte extended length)
I don't think the last one is used but having names for these would be handy. If I were to suggest some I'd probably go with:
PAYLOAD_LENGTH_TWO_BYTE = 126
PAYLOAD_LENGTH_EIGHT_BYTE = 127
MAX_PAYLOAD_LENGTH_SHORT = 125
MAX_PAYLOAD_LENGTH_TWO_BYTE = 2 ** 16
MAX_PAYLOAD_LENGTH_EIGHT_BYTE = 2 ** 64
I'm open to discussion though.
Because I'm a weirdo I've decided to protect the master branch and also to require code reviews and stuff (I know, what a killjoy), but that gets a bit tricky when the linter is failing. So this is a very minor refactor of the code to get the linter back to passing, without breaking tests.
Sometime in the future I'll try to start creeping the test coverage upwards too. =)