Please revert this change, it will be done as part of the release.

The issue is that those changes are necessary for this PR to work, since some of the new topics added to help require topics that aren't currently in there and so would be broken / hard to test. I can revert if the idea is that reviewers will re-generate that file on their own for testing, but just wanted to flag that the PR won't work as expected without it before actually rolling it back.

We do have some precedent for regenerating between releases (#153909, #144934, #125074). Is there any harm in doing it here, especially if the feature can't be used without it?

Regeneration at release is the standard process (it's automatically done by release-tools), it makes this easier to review IMO.

Right, but not regenerating makes this very difficult to test locally. How does this make it more difficult to review? Just ignore the regenerated file.

Since we don't verify it in the CI, it's another file that needs manual verification.

Sure, but manually verifying a generated file is much easier (and less harmful if we mess up) than letting a feature hit main and not work until release. Keep in mind that removing the generated file will also mean we have to remove a bunch of test coverage from this PR. We would, in essence, be trading the guarantee that the generated file is correct for the guarantee that the feature works correctly.

I'm inclined to cite "practicality beats purity" here. Yes, regenerating is a very minor breach of process, but I think reviewing this PR is much more difficult without the regenerated file, not vice versa.

My concern is more around security than correctness here (and not specific to this PR or its author). Since these generated files aren't verified in CI, changes to them in PRs introduce another potential attack surface that reviewers need to manually inspect.

Another downside of regenerating these files outside of releases is the potential for merge conflicts, though I don't think that's much of an issue in the case of pydoc.

That said, I understand the testing concerns, so if you'd like to keep the generated file in and verify the changes, I won't block on it.

Yeah, I'm not saying we should make this a common policy; I'm just arguing that we should make an exception for this PR. I'll make sure it's correct before landing this.

Or, we could verify pydoc_data as part of the "Check if generated files are up to date" CI check, assuming the generation is pretty fast.

urllib.parse.quote does extra things that seemingly don't work with the .html URL handler (such as converting ! to %21). I think we can switch to html.escape(name) if all we're after is the quote escaping?

I was actually originally worried about +, since I know that + is often interpreted as a space in a URL. So I leaned toward urllib.parse.quote just to be safe since I'm not sure what else might be special in the URL bar, and because urllib.parse.quote just felt more right to me in principle, based on the context. I also expect the difference to be largely invisible to the end user, other than slightly-grosser URLs if copy/pasting.

I hadn't noticed any broken pages in my original testing, though I did just now test searching for ! and indeed it shows a bad error message (FWIW, the error is not because of the escaping but because ! doesn't have an entry).

The change I just pushed tries to remedy that by unquoting the given URL on the error page (I was already unquoting when actually doing the lookup, so, e.g., searching for != should have worked before this most recent change).

If you'd prefer, though, I can switch it to use html.escape instead.

I think it would show up if you had a module with some weird Unicode characters in the name. I suppose another fix would be to call urllib.parse.unquote in get_html_page, if you prefer that.

I think I already have the unquote in the only path in get_html_page where these links could possibly lead (behind the check for if '=' in url, this line in the diff).

I could be missing something, though; is there a case that line doesn't cover that I'm not seeing?

Rather than an allowlist of things to skip, we shouldn't skip any module that is successfully imported. There are some legitimate names for modules that are dunders, such as __about__ or __init__. The rest of this PR looks good otherwise.