various cleanups for typing stuff #19270
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jelly
previously requested changes
Sep 1, 2023
|
The failure is tricky: we expect the In any case, that's not what I intended to do and it needs to be cleaned up. Yay for catching bugs? |
|
Indeed, the current behaviour of the Python bridge would cause problems with C peers, for example. |
allisonkarlitskaya
force-pushed
the
start-strict
branch
2 times, most recently
from
7fc5ddf
to
e600ebb
Compare
|
@allisonkarlitskaya @jelly seems this was very close to landing, and then slipped through the cracks 😢 It's conflicty, and by now I suppose it will also conflict with #19668. Moving to draft. |
Start building a list of files in src/cockpit which are clean under `mypy --strict` to ensure that we don't regress. The end goal is to have all of `src/cockpit` strictly typed.
At one point we wanted to implement this as a BufferedProtocol, where a memoryview would have likely been advantageous. We went with our current "dumb" bytes-based approach instead, and haven't found any egregious performance problems there, so let's remove the code that could theoretically deal with memoryview. Maybe we'll do something more intelligent at some point in the future. We also lift a check on the value being non-empty to the loop in the calling function. This is not strictly required, but speeds things up in the EOF case.
allisonkarlitskaya
force-pushed
the
start-strict
branch
from
e600ebb
to
ee953dc
Compare
This file is now clean under `mypy --strict`.
Since 747e7c2 we have the ability to send verbatim JSON objects as control messages, which we do when forwarding channel control messages to peers. Until now, we haven't applied the same handling for 'kill' messages because we haven't had access to the message object in order to be able to forward it. This means that we've been unintentionally breaking protocol: the current way we construct the control message from kwargs, we always send up sending both "host" and "group", even if one of them is `null`, which is a violation of protocol (these should either be strings, or absent). Let's wire the original message through and send it verbatim: this will help us in the next commit when we start enforcing type safety on kill message parameters.
Drop the manual error checking on 'init' messages and avoid the total lack of error checking for 'kill' and 'authorize' messages.
allisonkarlitskaya
force-pushed
the
start-strict
branch
from
ee953dc
to
d4bfd9a
Compare
martinpitt
approved these changes
Jan 31, 2024
Comment on lines
-118
to
-124
| # We know the length + newline is never more than 10 bytes, so just | ||
| # slice that out and deal with it directly. We don't have .index() on | ||
| # a memoryview, for example. | ||
| # From a performance standpoint, hitting the exception case is going to | ||
| # be very rare: we're going to receive more than the first few bytes of | ||
| # the packet in the regular case. The more likely situation is where | ||
| # we get "unlucky" and end up splitting the header between two read()s. |
There was a problem hiding this comment.
That comment is still current, other than the "we don't have .index" bit. But it's useful to explain the magic number 10 below, as well as splitting reads in between a number.
allisonkarlitskaya
dismissed
jelly’s stale review
This review is stale (requested changes already made)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This enables
mypy --stricton a handful of files. This was all fairly straight-forward.In general, we need to proceed from the bottom of the dependency tree upwards. That means that the next targets are:
We're stuck on config because systemd_ctypes dbus stuff isn't
--strictfriendly. We're stuck on router because the 'freeze' mechanism is pretty evil from a types perspective and I kind of want to get rid of it anyway.Let's get these "easy" ones in now.