Repository navigation
gh-113746: Disambiguate Documentation Search Results for Methods of set and frozenset - #138364
adqm wants to merge 4 commits into
Conversation
Sorry, something went wrong.
in both search results and the general index
AA-Turner
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
|
I'm certainly open to other alternatives here, but I couldn't find anything else that didn't involve a big restructuring of the stdtypes page and repeating a bunch of stuff between set and frozenset. I personally feel like this is misleading enough that it's worth having a band-aid in place until we have that better solution. 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: |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
Makes sense, thanks! |
Sorry, something went wrong.
This reverts commit c333b8f.
skirpichev
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
| 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.
I think you can revert this, as well as other cases, that reference common methods for set and frozenset.
Sorry, something went wrong.
|
+1 for this approach. It makes indexing better without damaging the text. |
Sorry, something went wrong.
This is an attempt to resolve the issue with search results for set.methodname turning up results like frozenset.methodname instead (initially described in #113746), which is especially misleading for methods that frozenset 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:
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 on library/stdtypes.html, I kept the old #frozenset.methodname anchors around even for methods that aren't actually implemented for frozensets, in the hopes of not breaking old links. One remaining weirdness is that the highlighting when navigating to one of the frozenset-based anchors doesn't match what we see when visiting a set-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/