bump pyright, introducing warnings for tons of missing docstrings#2910
bump pyright, introducing warnings for tons of missing docstrings#2910A5rocks merged 9 commits intopython-trio:masterfrom
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2910 +/- ##
=======================================
Coverage 99.64% 99.64%
=======================================
Files 117 117
Lines 17634 17643 +9
Branches 3172 3176 +4
=======================================
+ Hits 17572 17581 +9
Misses 43 43
Partials 19 19
|
|
Oh right, pyright doesn't exit with a failing status code on missing docstrings. Probably want to fix that too |
|
I think docstrings are near the bottom of things to worry about when running pyright on trio |
|
Indeed. Those public |
cool, yeah. Filtering out the ones that has docstrings at runtime the new count is 48 functions |
…to SocketType from stdlib
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
Should we split out filtering pyright issues so that doesn't have to depend on writing docstrings?
src/trio/_socket.py
Outdated
| raise NotImplementedError | ||
|
|
||
|
|
||
| # copy docstrings from socket.SocketType |
There was a problem hiding this comment.
This comment seems a tiny bit out of date. (given that the docstrings are copied from socket.socket or socket.SocketType, as is later mentioned)
filtering out errors a la #2959 falls in the same bin as filtering out docstring errors, so that's what I'm going for here. But yeah I'll open a separate issue for remaining docstrings that are missing, and have them tracked in a json/txt similar to before. |
|
I kind hate this script, but this should maybe sorta do the job and hopefully not be a complete pain in the ass to maintain.
sorta remaining todos depending on if anybody wants to put in the effort (please do add commits and/or have opinions!):
|
…nd stuff. Remove logic for 'not warned by pyright but missing docstring' cause that mostly shouldn't be a thing anymore
The "channels" and "streams" sections are mostly derived-class implementations of ABC methods, which I don't think need a docstring if their behavior is totally described by the ABC method's docstring. Maybe pyright has an option to allow that? |
pyright not having options is why this PR is needed in the first place 🙃 Lines 730 to 742 in 3b48476 |
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
I don't completely follow the logic for the new script but here's some mixed comments
| can resolve it, in order to check whether it has a `__doc__` at runtime and | ||
| verifytypes misses it because we're doing overly fancy stuff. | ||
| """ | ||
| assert trio.testing |
There was a problem hiding this comment.
Confused about the reason behind this assert
There was a problem hiding this comment.
We never directly use trio so isort wants to remove it, I had a comment to that effect previously but lost it for some reason.
Could maybe instead do it with some isort: skip, but that would also mess with import sorting.
| split_i = 1 | ||
| if name_parts[1] == "tests": | ||
| return True | ||
| if name_parts[1] in ("_core", "testing"): # noqa: SIM108 | ||
| split_i = 2 | ||
| else: | ||
| split_i = 1 |
There was a problem hiding this comment.
| split_i = 1 | |
| if name_parts[1] == "tests": | |
| return True | |
| if name_parts[1] in ("_core", "testing"): # noqa: SIM108 | |
| split_i = 2 | |
| else: | |
| split_i = 1 | |
| if name_parts[1] == "tests": | |
| return True | |
| split_to = 2 if name_parts[1] in ("_core", "testing") else 2 |
then split_i -> split_to?
There was a problem hiding this comment.
Also maybe assert name_parts[0] == "trio" at the top?
| if "AsyncIOWrapper" in str(exc) or name in ( | ||
| # Symbols not existing on all platforms, so we can't dynamically inspect them. | ||
| # Manually confirmed to have docstrings but pyright doesn't see them due to | ||
| # export shenanigans. TODO: actually manually confirm that. |
There was a problem hiding this comment.
We can probably add a test to make sure these symbols exist on these platforms: the issue then becomes how to keep things up to date.
I've added one possible idea as a diff.
There was a problem hiding this comment.
I currently don't care enough to dynamically validate these, so I propose we postpone that to a future time ™️
The two possible issues will be:
- We miss one object losing a docstring
- somebody encounters an error and needs to do manual intervention: this will happen anyway. The best way around that is to have sufficient amounts of comments that it's not super scary for newbies to do so.
| # darwin | ||
| "trio.lowlevel.current_kqueue", | ||
| "trio.lowlevel.monitor_kevent", | ||
| "trio.lowlevel.wait_kevent", | ||
| "trio._core._io_kqueue._KqueueStatistics", |
There was a problem hiding this comment.
Idea:
| # darwin | |
| "trio.lowlevel.current_kqueue", | |
| "trio.lowlevel.monitor_kevent", | |
| "trio.lowlevel.wait_kevent", | |
| "trio._core._io_kqueue._KqueueStatistics", | |
| # --- darwin | |
| "trio.lowlevel.current_kqueue", | |
| "trio.lowlevel.monitor_kevent", | |
| "trio.lowlevel.wait_kevent", | |
| "trio._core._io_kqueue._KqueueStatistics", | |
| # --- darwin |
Then the test splits the file on # --- darwin (or whatever platform its running), takes the middle, skipped_symbols = eval('[' + middle + ']'), and check those.
There was a problem hiding this comment.
Hrm, this starts feeling very silly that it's separate from test_static_tool_sees_class_members.
6 participants
With microsoft/pyright#6758 released, we now have warnings for missing docstrings on ... 22 classes and 133 functions.
A lot of them are sockets and path/fileIO, which we mostly can ignore. It's maybe time to write a program that filters the output, which would also enable us to run without
--ignoreexternal. At least the first ~40 should probably get some simple docstrings written for them though.