GeezerVison

Wow, what a massive amount of work!

That's over 300 queued CI jobs as well - I have never seen anything like it 😅. It's too bad that it's not possible to tell GitHub to build every commit in the PR, so one is instead forced to push each commit individually (which has side-effects).

That's over 300 queued CI jobs as well - I have never seen anything like it 😅.

Sorry--I should probably have checked with you first before pushing them that way, with this many commits!

No worries, I just wanted to comment on it because it's a first for me (at least in this scale), and it's great that GitHub can handle it :).

It looks like CodeQL complains starting in 29c63ac.

     @overload
-    def execute(self, command: Union[str, Sequence[Any]], *, as_process: Literal[True]) -> "AutoInterrupt":
+    def execute(
+        self,
+        command: Union[str, Sequence[Any]],
+        *,
+        as_process: Literal[True],
+    ) -> "AutoInterrupt":
         ...

That change, which reformats one of the Git.execute overloads, produces new warnings about the preexisting situations where CodeQL does not see redacted_command, cmdstr (derived from redacted_command), and the expression safe_decode(stderr_value) as safe.

Those uses are all down in the body of Git.execute, of course. I presume that, rather than having detected something new, CodeQL is redetecting what it has already detected. This actually makes sense: one of the @overload stubs that contains information about the arguments that can be passed to Git.execute, which can contain sensitive information and from which values are derived that CodeQL believes may not be sanitized, is now different code, so CodeQL is detecting the cause as a different (albeit equivalent) one.

This is to say that I think the CodeQL situation is analogous to that in #1813, even though it seems stranger here. If that is the case, then I think this is not cause for alarm. It seems unlikely it could be anything else, but I suggest verifying it by checking that corresponding warnings have also gone away, i.e., that CodeQL automatically dismisses equivalent warnings to each of the new ones.

It may be easier to examine once the CodeQL jobs have run for the last commit, and I expect automatic CodeQL comments to be posted as well, as in #1813.

This is incredible work! I don't even know how to express how incredible it is as I don't know how it's even humanly possible to read through this much code while doing all these small to bigger improvements, with most of them being related to something as abstract as whitespace.

This was actually faster than #1725, in part because of its narrower scope and my greater familiarity with the code, but in part because it required much less code reading, though I did have to read docstrings. The approach I took here was different from there, which is also why the individual commits here may not have been broken up quite as nicely: I did this in more of a breadth-first fashion, in multiple passes, some reading carefully and others skimming, and each time applying things I noticed on the previous pass, both about the documentation and about what I had caught myself missing.

The biggest whitespace changes were (a) in formatting of :role:/:role args...-led sections, which were very fast to do manually, and (b) wrapping, which was simpler than in #1725 because I was applying an 88-column wrap everywhere except where it clearly worsened readability, rather than trying to infer wrapping in each place from local context.

Because single newlines (in most places equivalent to a single space when rendered) often appear (and occasionally I even added them) to improve readability or ease of editing in the docstrings themselves, and also because the wrapping tool I used (the Rewrap extension in VS Code) doesn't automatically observe all reStructuredText syntax rules when wrapping Python docstring text, I wasn't able to just select entire docstrings and auto-wrap them in most cases. But I was able to select a sizable contiguous chunks of lines at a time and do it, which also aided in keeping my place and noticing other issues to fix.

I cannot imagine how a workflow around these changes must look like unless there is a way to automatically build and preview exactly the part of the file one is currently working on.

I don't have such a tool. VS Code renders docstrings when one hovers over the symbol, but it does so in kind of a fuzzy way that mostly interprets them as Markdown. I mostly worked in the docstring source itself, but I previewed changes with make -C doc html regularly. The Live Server extension for VS Code helped here, though I probably could've instead used file:// URLs and refreshed a lot.

As for my own review, I tried to use the renderered docs of the PR sporadically to get an idea of how it looks like. Nice initially, it quickly turned out that plenty of functions I would have expected to be findable through search simply weren't, so a preview wasn't really possible after all.

The API Reference is all on one page, so shortcomings in the provided search can sometimes be overcome by the web browser's own search (Ctrl+F for me). The first match will often be a mere mention of the symbol one seeks, but now that nearly all of these mentions link to the symbol, clicking it should be sufficient to get there. In hindsight I ought to have given that as the primary rationale for making internal class (and method) names links everywhere there was not strong reason not to do so: people often search for them and find something else that mentions them, and now they can usually click through immediately to what they are really looking for.

However, that the Read the Docs search--assuming I am right in thinking that this is what you mean--was not working as well as you expected deserves attention, and may be something we can fix. When looking into this, I found a problem which I hope to open a GitPython issue for soon. But I suspect it is different from what you are describing, because the problem I encountered does not affect searching in pull request builds. Can you say more about how you searched, and what you had expected would produce good results but did not?

[...] It would also be my expectation that IDEs can use these docstring links to allow the user to jump to the linked classes directly, or maybe they even render the docs in-line while resolving the links. [...]

I don't have this functionality in VS Code for Sphinx documentation in Python docstrings, though there may be extensions that provide it, or maybe there is even something I'm unaware of that I just need to configure (but nothing in a docstring is ever treated by my editor as a symbol). I suspect I would get it if using a more IDE-ish IDE, such as PyCharm (or Writerside, a documentation IDE I've been meaning to try).

However, symbol search is fast and accurate in VS Code for most Python projects, including GitPython, so I was able to look up symbols efficiently (including by selecting them in docstrings and pressing Ctrl+T).

Regarding #1850 (comment):

I was unable to reproduce the CodeQL check failure or alerts in my own fork, after trying CodeQL checks on both the push and pull_request triggers. It also somehow went away here as of ee0301a. I don't know of any reason the content of that commit would ameliorate or otherwise affect CodeQL alerts.

Regarding #1850 (comment):

It's too bad that it's not possible to tell GitHub to build every commit in the PR, so one is instead forced to push each commit individually

I wonder if there is some way to copy the status from the corresponding run in a fork that has already occurred.

(which has side-effects).

Are there side effects of repeated pushes that I should be aware of, other than more entries in the remote (and local) reflog?

Can you say more about how you searched, and what you had expected would produce good results but did not?

I pasted the name of two functions, the second one I forgot but it was public (and pretty long), and the first one was refresh(). There is two of these, and it only finds one of them which was initially confusing as the text didn't match.
The other public method I searched for it didn't find at all. The browser search I tried to use, but it didn't work well for me.

Overall, I find the API documentation quality on a level of 'better than nothing', but it always was like that and by now I suppose I am spoiled by rustdoc.

I wonder if there is some way to copy the status from the corresponding run in a fork that has already occurred.

This would be very clever, and could save a lot of compute as well!

Are there side effects of repeated pushes that I should be aware of, other than more entries in the remote (and local) reflog?

I get one email per push, in the moment the push is received😅. It's fine though, my email is setup none-invasively to not break focus.

I pasted the name of two functions, the second one I forgot but it was public (and pretty long), and the first one was refresh(). There is two of these, and it only finds one of them which was initially confusing as the text didn't match.

When I search for refresh or refresh(), whether locally, hosted by Read the Docs for a PR, or hosted by Read the Docs for the tip of the main branch (i.e., "latest"), it finds both Git.refresh and FetchInfo.refresh. The one people should actually use has not been emitted at all, as noted in #1854, but it should be visible in the PR preview build for #1855 (and in "latest" if that PR is merged).

The other public method I searched for it didn't find at all. The browser search I tried to use, but it didn't work well for me.

I have had some trouble with some other search terms. For example, TagObject doesn't always turn up particularly useful results. I still haven't reproduced the details of this and opened an issue for it, but when I do maybe that will help lead to an improvement.

I get one email per push, in the moment the push is received😅. It's fine though, my email is setup none-invasively to not break focus.

I was unaware that notifications were generated for PRs that are opened as drafts and still marked draft!

It's fine though, my email is setup none-invasively to not break focus.

I have taken this to mean that it is fine for me to continue pushing commits in the way I have been doing (in cases where they would otherwise not have CI check statuses). However, if that is not the case and you'd prefer I stop doing that, you can let me know.