Add missing docstrings #2762

jakkdl · 2023-08-17T12:42:58Z

I started trying to write these ... and while I could maybe figure out what the functions are supposed to do it would be lovely if somebody else chipped in with suggestions (@A5rocks? @njsmith?)

This also made me notice that there were some problems in #2753 with open_process and run_process, I noted them here but they should probably be fixed by @TeamSpen210 in a separate PR

codecov · 2023-08-17T12:46:14Z

Codecov Report

Merging #2762 (218c13d) into master (315a0e8) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2762   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         115      115           
  Lines       17551    17551           
  Branches     3149     3149           
=======================================
  Hits        17408    17408           
  Misses         99       99           
  Partials       44       44

Files	Coverage Δ
src/trio/_core/_io_epoll.py	`100.00% <ø> (ø)`
src/trio/_core/_io_kqueue.py	`87.20% <ø> (ø)`
src/trio/_core/_io_windows.py	`98.80% <ø> (ø)`
src/trio/_socket.py	`100.00% <ø> (ø)`
src/trio/_subprocess.py	`98.27% <ø> (ø)`
src/trio/tests.py	`100.00% <ø> (ø)`

jakkdl · 2023-09-26T07:55:56Z

This is now the only thing remaining for pyright --verifytypes to be run without any issues, and make it possible to remove the endlessly infuriating trio/_tests/verify_types_*.json files. Would love somebody to take a stab at it, if any suggested docstrings are incorrect somebody will probably come along and correct them in a review anyhow.

…l.rst, and copy documentation from reference-lowlevel.rst to trio/_core_io kqueue, epoll and windows

…xports when getting ruff warnings, add docstring for fromshare

…the value of success on AssertionError

jakkdl · 2023-10-09T15:12:08Z

need to check if docstrings being undefined when TYPE_CHECKING==True is a problem for any tools.
- especially for the overload/non-overload diff
- @TeamSpen210 why does run_process and open_process have @overload on unix but not windows?
I had to modify trio/_tools/gen_exports because I was getting warnings:

Warnings when running ruff:
warning: Selection of nursery rule `E225` without the `--preview` flag is deprecated.
warning: Selection of nursery rule `E251` without the `--preview` flag is deprecated.
warning: Selection of nursery rule `E113` without the `--preview` flag is deprecated.

and my change broke the gen_exports ruff test. @CoolCat467:
1. why are we getting that warning (is something weirdly configured?)
2. result.returncode != 0 does not trigger for some reason, which I fail to reproduce in my shell where bad input to python -m ruff gives $? == 1 np.
pre-commit.ci is failing ... and ruff is saying there are 7 problems, but not saying what they are. @CoolCat467 something needs to be changed here
- it's also weird that pre-commit is passing on my local machine, but failing in CI.

jakkdl · 2023-10-09T15:17:14Z

@njsmith (and anybody else even remotely familiar with #26, #52 or the docstrings in question):

I copied docstrings from reference-lowlevel.rst, which was quite straightforwardly the right thing to do for wait_readable, wait_writable, and notify_closing.
Less obvious what to do with current_kqueue and friends from Windows: improve usability for low-level IOCP operations #26 and Windows event notification #52, ended up just coping the disclaimer.
for some reason write_overlapped and readinto_overlapped was missing entirely from the documentation. I tentatively added them where I'm guessing they should be, if anywhere.

CoolCat467 · 2023-10-10T01:58:41Z

why are we getting that warning (is something weirdly configured?)

I think this might be because of differences in the installed version of ruff and the pre-commit ruff hook version, not completely sure to be honest though. At least for me, I always have to run the gen_exports script with python3.11 instead of just running it directly, because my system defaults to python3.9, and the version of packages used in the different python installations is different. Not sure if this applies but I thought I'd mention it.

pre-commit.ci is failing ... and ruff is saying there are 7 problems, but not saying what they are. @CoolCat467 something needs to be changed here

This is because it's attempting to autofix the errors, and right now the config says it doesn't need to comment about what fixes it applies.
--fix-only says:

Fix any fixable lint violations, but don't report on leftover violations. Implies --fix.

So I think the "leftover violations" are the warnings, or maybe we need to specifically say --no-preview

jakkdl · 2023-10-10T09:47:34Z

why are we getting that warning (is something weirdly configured?)

I think this might be because of differences in the installed version of ruff and the pre-commit ruff hook version, not completely sure to be honest though. At least for me, I always have to run the gen_exports script with python3.11 instead of just running it directly, because my system defaults to python3.9, and the version of packages used in the different python installations is different. Not sure if this applies but I thought I'd mention it.

Ah shit, this is my bad. I had a ruff.toml lying around from when I was running ruff locally myself before it was added to pyproject.toml.

pre-commit.ci is failing ... and ruff is saying there are 7 problems, but not saying what they are. @CoolCat467 something needs to be changed here

This is because it's attempting to autofix the errors, and right now the config says it doesn't need to comment about what fixes it applies. --fix-only says:

Fix any fixable lint violations, but don't report on leftover violations. Implies --fix.

So I think the "leftover violations" are the warnings, or maybe we need to specifically say --no-preview

Yeah, it's autofixing instead of reporting the errors but we've disabled autofix in the pre-commit CI and it's failing because files are getting changed. So we either need to disable autofix for ruff when it's run through pre-commit, or for pre-commit to display the changes to the files that caused it to fail.

Re: "@CoolCat467
Success should still be false for warnings"
my change was specifically to not have it fail when giving warnings, but given I've resolved that issue we can probably revert to what the file was like originally and have if result.returncode != 0 or result.stderr:. Though it might be more robust to return return_value, stdout, stderr, the tool itself will ignore any warnings on stderr and only fail if return value is non-zero, and the fail test will assert that return value is non-zero.
This is probably a better fit in a separate PR though, and in here I can revert all changes to [test_]gen_exports.

TeamSpen210 · 2023-10-10T10:48:26Z

I think I got the overloads from the trio_typing stubs. The difference is that on Linux/Mac, you can pass a string command only with shell=True, while on Windows you can pass either a string or list of strings regardless of the option. You can't use a single overload for a function, so I had to do the _open_process business. Looking at the typeshed stubs they don't make this distinction for Popen etc, maybe we should too and just simplify the mess.

jakkdl · 2023-10-10T11:01:23Z

I think I got the overloads from the trio_typing stubs. The difference is that on Linux/Mac, you can pass a string command only with shell=True, while on Windows you can pass either a string or list of strings regardless of the option. You can't use a single overload for a function, so I had to do the _open_process business. Looking at the typeshed stubs they don't make this distinction for Popen etc, maybe we should too and just simplify the mess.

Simplifying the mess sounds like a big win to me, cause this is indeed really really messy

Zac-HD

Looks great to me - thanks, @jakkdl!

CoolCat467

I think that for the information we have these look great

A5rocks

General formatting things, content looks fine.

trio/_core/_io_epoll.py

A5rocks · 2023-11-05T23:46:08Z

trio/_core/_io_epoll.py

+        """Call this before closing a file descriptor (on Unix) or socket (on
+        Windows). This will cause any `wait_readable` or `wait_writable`
+        calls on the given object to immediately wake up and raise
+        `~trio.ClosedResourceError`.


First line should be a description but not exactly sure what would work.

Notify waiters of the given object that it will be closed.

works?

trio/_core/_io_kqueue.py

trio/_core/_io_windows.py

A5rocks · 2023-11-05T23:50:25Z

trio/_core/_io_windows.py

+        """Call this before closing a file descriptor (on Unix) or socket (on
+        Windows). This will cause any `wait_readable` or `wait_writable`
+        calls on the given object to immediately wake up and raise
+        `~trio.ClosedResourceError`.


Also missing summary.

A5rocks · 2023-11-05T23:51:22Z

trio/_subprocess.py

+            Raises:
+              OSError: if the process spawning fails, for example because the
+                 specified command could not be found.


Is this docstring format correct? You use the plain sphinx format elsewhere in this PR.

This is a copy-paste of the docstring at line 346 for _open_process - but maybe that one is incorrectly formatted? Or all the other ones I copy-pasted from the docs are?

TBH as long as sphinx renders this fine then whatever. And it does seem to.

A5rocks · 2023-11-05T23:51:50Z

trio/_subprocess.py

+            Raises:
+              UnicodeError: if ``stdin`` is specified as a Unicode string, rather
+                  than bytes
+              ValueError: if multiple redirections are specified for the same
+                  stream, e.g., both ``capture_stdout=True`` and
+                  ``stdout=subprocess.DEVNULL``
+              subprocess.CalledProcessError: if ``check=False`` is not passed
+                  and the process exits with a nonzero exit status
+              OSError: if an error is encountered starting or communicating with
+                  the process


Again note about docstring format

same thing - copypasted from run_process earlier in the file. (and yes duplicate docstrings is incredibly ugly and bad... but I don't know how to improve @TeamSpen210's magic to avoid needing to and have it work both on runtime, in static analysis, and on all platforms.)

TBH sphinx renders this fine so whatever.

Co-authored-by: EXPLOSION <git@helvetica.moe>

A5rocks

Looks fine, if things don't like multiple docstring formats then we can fix them then. Not worth the time ATM I think.

(also the summary looks fine, so maybe add that before merging ^^ -- not sure if you already did but the comments aren't resolved so IDK)

jakkdl · 2023-11-12T14:34:07Z

Sorry yeah, I added them :)

Time to get rid of the jsons!! 🚀

add TODO markers for all missing docstrings

423864e

jakkdl mentioned this pull request Aug 17, 2023

Remaining tasks until --verifytypes passes and check_type_completeness.py can be removed #2734

Closed

41 tasks

Merge branch 'master' into missing_docstrings

6b31b77

jakkdl mentioned this pull request Aug 21, 2023

check type completeness on all platforms #2767

Merged

jakkdl added polish docs labels Sep 13, 2023

Merge remote-tracking branch 'origin/master' into missing_docstrings

e378d4d

jakkdl added 5 commits September 26, 2023 11:40

re-add placeholder docstrings lost when merging

6dd5753

fix weird discrepancy in verify_types_*.json

5e48b18

add missing functions from trio/_core_io_windows to reference-lowleve…

148f77d

…l.rst, and copy documentation from reference-lowlevel.rst to trio/_core_io kqueue, epoll and windows

Merge remote-tracking branch 'origin/master' into missing_docstrings

213e6fa

copy docstrings for open_process and run_process, don't abandon gen_e…

f304740

…xports when getting ruff warnings, add docstring for fromshare

jakkdl marked this pull request as ready for review October 9, 2023 14:42

print warning to stderr, undo removal of --output-format=text, print …

73e3982

…the value of success on AssertionError

Update ruff version and regenerate again

6a2eb95

CoolCat467 added 4 commits October 9, 2023 21:02

Update ruff requirement

ef7f02b

Success should still be false for warnings

c0875c3

Have asserts show error response when not success

330a51d

Update verify_types.json

956ec5c

jakkdl changed the title ~~[WIP] Add missing docstrings~~ Add missing docstrings Oct 25, 2023

CoolCat467 and others added 2 commits October 31, 2023 15:28

Merge branch 'master' into missing_docstrings

e5195bb

Merge branch 'master' into missing_docstrings

c26d84b

undo changes to gen_exports

340868a

jakkdl requested review from A5rocks, TeamSpen210, njsmith and Zac-HD November 2, 2023 14:33

Zac-HD approved these changes Nov 4, 2023

View reviewed changes

CoolCat467 approved these changes Nov 4, 2023

View reviewed changes

A5rocks requested changes Nov 5, 2023

View reviewed changes

jakkdl and others added 5 commits November 11, 2023 12:54

Apply suggestions from code review

fb16d10

Co-authored-by: EXPLOSION <git@helvetica.moe>

update generated files

34cdead

Merge remote-tracking branch 'origin/master' into missing_docstrings

1a3cc83

Merge remote-tracking branch 'origin/master' into missing_docstrings

2df6af4

add summary to notify_closing

218c13d

jakkdl requested a review from A5rocks November 11, 2023 12:06

CoolCat467 approved these changes Nov 11, 2023

View reviewed changes

A5rocks approved these changes Nov 11, 2023

View reviewed changes

jakkdl merged commit 44f7f2d into python-trio:master Nov 12, 2023

jakkdl deleted the missing_docstrings branch November 12, 2023 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing docstrings #2762

Add missing docstrings #2762

jakkdl commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

jakkdl commented Sep 26, 2023

jakkdl commented Oct 9, 2023

jakkdl commented Oct 9, 2023

CoolCat467 commented Oct 10, 2023 •

edited

Loading

jakkdl commented Oct 10, 2023 •

edited

Loading

TeamSpen210 commented Oct 10, 2023 •

edited

Loading

jakkdl commented Oct 10, 2023

Zac-HD left a comment

CoolCat467 left a comment

A5rocks left a comment

A5rocks Nov 5, 2023

jakkdl Nov 11, 2023

A5rocks Nov 11, 2023

A5rocks Nov 5, 2023

A5rocks Nov 5, 2023

jakkdl Nov 11, 2023

A5rocks Nov 11, 2023

A5rocks Nov 5, 2023

jakkdl Nov 11, 2023

A5rocks Nov 11, 2023

A5rocks left a comment •

edited

Loading

jakkdl commented Nov 12, 2023

Add missing docstrings #2762

Add missing docstrings #2762

Conversation

jakkdl commented Aug 17, 2023 • edited Loading

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

jakkdl commented Sep 26, 2023

jakkdl commented Oct 9, 2023

jakkdl commented Oct 9, 2023

CoolCat467 commented Oct 10, 2023 • edited Loading

jakkdl commented Oct 10, 2023 • edited Loading

TeamSpen210 commented Oct 10, 2023 • edited Loading

jakkdl commented Oct 10, 2023

Zac-HD left a comment

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

A5rocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A5rocks left a comment • edited Loading

Choose a reason for hiding this comment

jakkdl commented Nov 12, 2023

jakkdl commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

CoolCat467 commented Oct 10, 2023 •

edited

Loading

jakkdl commented Oct 10, 2023 •

edited

Loading

TeamSpen210 commented Oct 10, 2023 •

edited

Loading

A5rocks left a comment •

edited

Loading