add more comprehensive typing and check with mypy #144
Conversation
terencehonles
changed the title
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
2 times, most recently
from
8e1812e to
db5c90d
Compare
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
db5c90d to
a23627a
Compare
|
Ok, that should be back in a state that the CI can run against it |
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
a23627a to
f9b326b
Compare
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
f9b326b to
0c44dbe
Compare
| super().__init__(*args, **kwargs) | ||
| self.log_group = log_group | ||
| self.stream_name = stream_name | ||
| self.use_queues = use_queues | ||
| self.send_interval = send_interval | ||
| if isinstance(send_interval, timedelta): |
There was a problem hiding this comment.
This was updated to match the text, but the :type: was previously just int
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
0c44dbe to
4785a38
Compare
terencehonles
changed the title
|
I'll need to double check everything, but I've resolved the conflicts and that should be closer to what I was trying to merge. I didn't type new functions that were added in #158, but this at least gets this PR into a state where it's more easily comparable to |
docs/conf.py
Outdated
| @@ -11,6 +11,8 @@ | |||
| source_suffix = [".rst", ".md"] | |||
| exclude_patterns = ["_build", "Thumbs.db", ".DS_Store"] | |||
| pygments_style = "sphinx" | |||
| autoclass_content = "both" | |||
| #autodoc_typehints = "description" | |||
There was a problem hiding this comment.
This was for sphinx.ext.autodoc.typehints I assume sphinx_autodoc_typehints doesn't need this, but not sure why the other project was chosen and it looks like they are looking for a new maintainer and it may make sense to go with the main Sphinx project
There was a problem hiding this comment.
Thanks for the pointer. I've addressed this separately in ed0d3cc.
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
2 times, most recently
from
8912f24 to
651f4da
Compare
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
651f4da to
7e51bb0
Compare
terencehonles
force-pushed
the
drop-eol-python-3.5
branch
from
7e51bb0 to
92e4e7c
Compare
kislyuk
force-pushed
the
develop
branch
2 times, most recently
from
f78aeb3 to
256fe2a
Compare
| warnings.warn("Log message size exceeds CWL max payload size, truncated", WatchtowerWarning) | ||
| _msg2["message"] = _msg2["message"][:max_message_body_size] | ||
| return _msg2 | ||
|
|
||
| # See https://boto3.readthedocs.io/en/latest/reference/services/logs.html#CloudWatchLogs.Client.put_log_events | ||
| while msg != self.END: | ||
| cur_batch = [] if msg is None or msg == self.FLUSH else [msg] | ||
| cur_batch: List[dict] = [] if msg is None or msg == self.FLUSH else [cast(dict, msg)] |
There was a problem hiding this comment.
This looks almost all good to go, except let's please remove all of the casts and asserts. The typing information should decorate the function signatures, not be part of the execution. If mypy fails to check this method without calls to cast and asserts, then we should disable mypy checking for those lines, or possibly split up the method into multiple methods.
There was a problem hiding this comment.
I'll look at the PR to see what makes sense. In case you aren't familiar, cast is just a noop at runtime so while it's calling more code it shouldn't be that big of a deal. It does enrich mypy with more information and I'm not sure if mypy treats a type: ignore equally, but I can double check. For the asserts I believe I have generally used them to narrow optional variables, but I will review the PR since it's been awhile since I looked at this code.
Sorry I've been pretty busy focused on other things. The branch had already been updated to include at least some typing and all that needed to be clicked is the "reopen" button. Since that may not have been clear I'm reopening #137 as a new PR.