-
-
Notifications
You must be signed in to change notification settings - Fork 33k
gh-113746: Disambiguate Documentation Search Results for Methods of set
and frozenset
#138364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
in both search results and the general index
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need NEWS here.
I'm not massively comfortable adding a custom extension just for search results, there must be a better way here.
A
I'm certainly open to other alternatives here, but I couldn't find anything else that didn't involve a big restructuring of the I can roll back the NEWS entry if you want, but it feels to me like whatever fix does end up going in here is fixing a notable bug that's worth a mention. For information's sake, a couple of illustrative changes: |
As a rule of thumb, documentation changes don't get NEWS entries. The exception is changes to requirements for building the documentation, as they're visible to downstream redistributors. A |
Makes sense, thanks! |
This reverts commit c333b8f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there must be a better way here.
How you might be sure without any concrete proposal?
I think it's not a reason to entirely block a pr, which is in a good shape and solves, IMO, an important issue. @AA-Turner, please reconsider. At least, it can be viewed as a temporary solution, as documentation is not changed.
Alternative suggestion from closed issue: #114336 (comment) It could be more generic and useful in other places of the stdlib, but it doesn't solve e.g. anchor issue (I think we want reference common methods both as #set.foo
and #frozenset.foo
).
PS: @adqm, it seems you miss change in Doc/whatsnew/2.3.rst.
CC @rhettinger
abstract methods: :meth:`~object.__contains__`, :meth:`~container.__iter__`, and | ||
:meth:`~object.__len__`. The ABC supplies the remaining methods such as | ||
:meth:`!__and__` and :meth:`~frozenset.isdisjoint`:: | ||
:meth:`!__and__` and :meth:`~set.isdisjoint`:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can revert this, as well as other cases, that reference common methods for set and frozenset.
+1 for this approach. It makes indexing better without damaging the text. |
This is an attempt to resolve the issue with search results for
set.methodname
turning up results likefrozenset.methodname
instead (initially described in #113746), which is especially misleading for methods thatfrozenset
does not support (e.g.,frozenset.update
). It also adjusts the general index, which had similar problems.Honestly, this patch feels quite hacky to me, but it does improve the search results so that:
set
andfrozenset
now show up separately for each type in the search results and the general index, andfrozenset
doesn't support don't show up if searching forfrozenset.methodname
.In contrast to #130822, this also intentionally preserves the text on the page exactly (I agree that the current organization is nice).
Despite the new
#set.methodname
anchors onlibrary/stdtypes.html
, I kept the old#frozenset.methodname
anchors around even for methods that aren't actually implemented forfrozenset
s, in the hopes of not breaking old links. One remaining weirdness is that the highlighting when navigating to one of thefrozenset
-based anchors doesn't match what we see when visiting aset
-based anchor. I wasn't able to resolve that, but I think it's minor compared to the gains here.I tried quite a few other approaches that didn't quite work out, but I still expect that there may be a better way (I've never really worked with Sphinx before); if so, I'd be happy to hear suggestions! 🙂
📚 Documentation preview 📚: https://cpython-previews--138364.org.readthedocs.build/