Skip to content
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

gh-106318: Add examples for the str methods in collapsible sections #111743

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

adorilson
Copy link
Contributor

@adorilson adorilson commented Nov 4, 2023

This PR complements #105670, turning the examples into collapsible sections, as suggested in #106318.

I would like a "go ahead" to continue the work.


📚 Documentation preview 📚: https://cpython-previews--111743.org.readthedocs.build/

Copy link
Contributor

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed up until format_map, since beyond the PR doesn't yet use details

Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Doc/library/stdtypes.rst Outdated Show resolved Hide resolved
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the issue, I don't think using .. raw:: is a good approach.

An "Examples" section at the bottom would IMHO be better, and could also combine related functions (e.g. upper/lower/title/capitalize/casefold) like I did in the str.format docs.

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Jan 23, 2024

Shipping these examples somewhere else would be worse for users IMO. If I want to look up what str.maketrans does, I'm going to look at the str.maketrans docs, not read through the whole page

In these discussion about .. raw:: I'm having a hard time understanding whether objections are fundamental to the proposal or whether they're predicated on sphinx implementation issues. If we had a magic sphinx wand, are inline collapsible sections something you (or other docs-community folks) would be okay with or not?

@ezio-melotti
Copy link
Member

If I want to look up what str.maketrans does, I'm going to look at the str.maketrans docs, not read through the whole page

This is a discoverability problem, but it can be easily be solved by turning the "See examples" text that expands the collapsed section in a link to the bottom of the page, where the section with all the examples is.

In these discussion about .. raw:: I'm having a hard time understanding whether objections are fundamental to the proposal or whether they're predicated on sphinx implementation issues.

There are a few issues at different levels:

  • .. raw:: itself is not an elegant solution implementation-wise, since it requires hardcoding raw HTML and also leads to repetition and increases the risk of errors (I'm not sure Sphinx is able to check markup errors in raw directives).
  • using existing plugins solves these problems, but introduces a new issue: the doc should be buildable without third-part extensions.
  • both these solutions, afaik, have the additional problem that they only works with the HTML builder, and not with the other builders -- something that we also want to support (adopting graceful degradation is an acceptable compromise here).

If we had a magic sphinx wand, are inline collapsible sections something you (or other docs-community folks) would be okay with or not?

If all the problems listed above were magically solved (i.e. if Sphinx added a built-in support for collapsible sections that words with all builders), then they could definitely be a useful tool. They still have some minor issues (e.g. iirc ctrl+f doesn't detect text within a collapsed section and they might be easy to miss while skimming through a page), but at that point the pros might outweigh the cons.

Finally, there's also another higher-level and more general discussion that we should have (probably not here, the next doc meeting might be better): should we have some convention on where to put examples?

We don't want to end up in a situation where a page uses collapsible sections, another has examples at the top, another at the bottom, another in the middle, another in a separate page, etc. If we agree on some conventions, it will also be easier for users to set their expectations and know where to look.

.. raw:: html

</details>
</dd>


.. _meth-str-join:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a useful directive? If yes, can I add to other methods in this PR or another?

@ezio-melotti
Copy link
Member

Finally, there's also another higher-level and more general discussion that we should have (probably not here, the next doc meeting might be better): should we have some convention on where to put examples?

We discussed this during the last meeting, and we agreed that a better approach would be to have a couple of simpler/short examples inline (i.e. just after the fuction doc) and possibly link to an "Example" section with more examples at the bottom. If we follow this approach, we don't need to worry about having collapsible sections and the issues that come along with those.

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Feb 11, 2024

Great! The examples in this PR are all simple and short.

So it sounds like we should just keep these in line, without the collapsible sections. And as per your last comment, if Sphinx develops better collapsible section tech in the future, we can incorporate this (and I'm happy to do the work for this myself).

In case folks read this thread later want a concrete example of what that would look like, here's the equivalent Go docs with collapsible sections: https://pkg.go.dev/strings#pkg-functions (to keep in theme with the Rob Pike quote from above )

@adorilson
Copy link
Contributor Author

So it sounds like we should just keep these in line, without the collapsible sections. And as per your last comment, if Sphinx develops better collapsible section tech in the future, we can incorporate this (and I'm happy to do the work for this myself).

Do you mean reject this PR in favor of #105670 ?

@ezio-melotti
Copy link
Member

Do you mean reject this PR in favor of #105670 ?

Are those the same examples, just without collapsible sections?

@adorilson
Copy link
Contributor Author

Do you mean reject this PR in favor of #105670 ?

Are those the same examples, just without collapsible sections?

Exactly. For this PR, I only forked that branch and added the collapsible sections.

@hugovk hugovk removed the needs backport to 3.11 only security fixes label Apr 11, 2024
@serhiy-storchaka serhiy-storchaka added the needs backport to 3.13 bugs and security fixes label May 9, 2024
@blaisep
Copy link
Contributor

blaisep commented May 22, 2024

OK, I have been reviewing "simple" PR docs and it seems like the consensus is to use #105670 and then close this one ( #111743 )

@adorilson , if you agree, please close this PR.

@hauntsaninja
Copy link
Contributor

hauntsaninja commented May 22, 2024

Silly question, where is the consensus?

@blaisep
Copy link
Contributor

blaisep commented May 22, 2024

Silly question, where is the consensus?

Well, it seems that @adorilson , @ezio-melotti , @hugovk , @terryjreedy , @CAM-Gerlach , @encukou all agree that the examples are useful and that we should use them without the additional formatting.
@hauntsaninja , do you have an objection to adding the examples?

@hauntsaninja
Copy link
Contributor

hauntsaninja commented May 23, 2024

I'm strongly in favour of adding the examples, see e.g. #111743 (review) and my other reviews on this and related PRs

But last I checked, Ezio and others weren't sure what the best way and place and format to add them was. I still can't find discussion that comes to a consensus on this (i.e. to go with #105670) and was curious, which is why I asked :-)

(Personally, I'm fine with any option that adds the examples, with a very mild preference for collapsible sections)

@adorilson
Copy link
Contributor Author

But last I checked, Ezio and others weren't sure what the best way and place and format to add them was. I still can't find discussion that comes to a consensus on this (i.e. to go with #105670) and was curious, which is why I asked :-)

In fact, maybe "consensus" is a strong word.
But I understood that adding examples without collapsible sections was a decision, and @ezio-melotti would review #105670 .

@blaisep
Copy link
Contributor

blaisep commented May 24, 2024

I see. Well, I think the examples are very good, so I made a bit of a compromise by trying to integrate them more closely with the prose they describe, Doc/library/stdtypes.rst
My motivation is to expedite getting these examples into the docs.
If PR #119445 is not useful, I'll close it.

@adorilson
Copy link
Contributor Author

Hi, @blaisep.

I've seen your changes on my PR. And I like it.

I'll keep this PR open until some other related PR is merged. But if some core developers want to close it, go ahead. I think this (and also #105670 ) can be closed in favour of #119445.

@picnixz
Copy link
Contributor

picnixz commented May 28, 2024

By the way, we are currently in the process of reviving sphinx-doc/sphinx#10532 which could make this PR much cleaner in the future. I cannot give you a merge ETA because Adam and Chris seem quite busy but I personally hope that we can make it for the next Sphinx release (or the one after).

EDIT: I know that this PR was already mentioned but since it was in January and we had some activity in April, I wanted to mention that we did not forget the use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants