Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2704 +/- ##
=======================================
Coverage 98.88% 98.89%
=======================================
Files 113 113
Lines 16483 16490 +7
Branches 3000 3000
=======================================
+ Hits 16300 16307 +7
Misses 126 126
Partials 57 57
|
|
Exposed and renamed |
jakkdl
force-pushed
the
typing_improvements_dtls
branch
from
1af492f to
f08e848
Compare
|
Oops, forgot to push yesterday. But merged in |
|
|
||
| See :meth:`~Nursery.start`. | ||
|
|
||
| .. autoclass:: TaskStatus |
There was a problem hiding this comment.
https://trio--2704.org.readthedocs.build/en/2704/reference-core.html#trio.TaskStatus doesn't seem very useful, add :members:?
There was a problem hiding this comment.
It's ... still not very useful. The class needs a docstring and the started needs one too.
|
|
||
| .. automethod:: statistics | ||
|
|
||
| .. autoclass:: DTLSChannelStatistics |
There was a problem hiding this comment.
https://trio--2704.org.readthedocs.build/en/2704/reference-io.html#trio.DTLSChannel.statistics looks like we might want to de-duplicated these docs a bit; make the first a cross-reference to the newly-added second?
There was a problem hiding this comment.
Should do the same with the other *Statistics.
| [[tool.mypy.overrides]] | ||
| module = [ | ||
| "trio._dtls", |
There was a problem hiding this comment.
I usually prefer default-strict, allowlist relaxed; but in this case I think we should continue doing it the other way for a bit longer.
Unrelated, we can delete the Pytest warnings-ignores a bit further down since we're now depending on the fixed version.
There was a problem hiding this comment.
yeah, was planning to switch over once the allowlist got .. not huge.
I'll open a separate PR with removing warnings.
|
|
||
| Currently this has only one attribute: | ||
|
|
||
| - ``incoming_packets_dropped_in_trio`` (``int``): Gives a count of the number of |
There was a problem hiding this comment.
I think that Sphinx will do the right thing if we move this description to an attribute-docstring below, and set :members: in the .rst file.
There was a problem hiding this comment.
It seems that should work, either with :members: or with :autoattribute: but I seem to fail to find the right magical invocation to get it to work and print the attribute docstring. I've tried setting the attribute-docstring both with #: on the line before, or with a """ docstring afterwards. Anybody else willing to play around with it is very welcome to.
https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-autoattribute
https://stackoverflow.com/questions/22717179/sphinx-class-attribute-documentation
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
This seems fine. Just a couple minor nitpicks. Feel free to merge! (Maybe wait a day to see if anyone chimes in?)
trio/_dtls.py
Outdated
| record: Record | ||
|
|
||
|
|
||
| # Needs Union until <3.10 is dropped |
There was a problem hiding this comment.
FWIW I don't think this comment is necessary, as long as we remember to run pyupgrade (or manually ctrl+f for unions) we'll manage. I guess this is up to you though.
There was a problem hiding this comment.
There's another comment like this in another file (forgot its name and forgot to comment when I saw it), so above applies to that too.
There was a problem hiding this comment.
that one spawned enough confusion (mostly in me probably) that I ended up writing a proper comment, so I don't think leaving that one hurts.
|
Eh, they've had two weeks to chime in and it's blocking moar typing PRs - so I'll merge. |
4 participants
The remaining entries in
verify_types.jsonfor classes in_dtlsis due to_SocketTypenot being fully typed yet.