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

Reorganise the devguide to directories #901

Merged
merged 21 commits into from
Jul 11, 2022
Merged

Conversation

AA-Turner
Copy link
Member

From the discord discussion, this is an attempt to add some structure to Docs/Tests/Triage and clean up the top level.

No content has been edited, except for combing two duplicated "how to become a triager" sections. As I moved content between pages I had to edit the underlines -- I also standardised some ^ underlines and out-of-order underlines to allow easier page moves in the future (e.g. a core-dev folder, buildbots, language design and change).

cc: @ezio-melotti @encukou @CAM-Gerlach

Preview: https://cpython-devguide--901.org.readthedocs.build/

Follows on from #895 where things went catastrophically wrong.

A

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jun 17, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@CAM-Gerlach CAM-Gerlach requested review from CAM-Gerlach, encukou, brettcannon and ezio-melotti and removed request for pablogsal June 17, 2022 17:45
@brettcannon brettcannon removed their request for review June 17, 2022 19:08
@brettcannon
Copy link
Member

FYI I have no opinion on this, so I'm dropping my review request.

@CAM-Gerlach
Copy link
Member

FYI, just in time for this PR, GitHub (finally) implemented the ability to seamlessly view commit history across file renames and moves.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks again, @AA-Turner ! LGTM, aside from a few minor issues.

  • I grepped and found hardcoded external links to devguide.python.org three places (which are now outdated), which should be replaced with :ref: and any links using them updated:

    • core-developers/committing.rst:231:.. _`Python Triage Team`
    • getting-started/getting-help.rst:29: `Discourse <https://devguide.python.org/communication/#discourse-discuss-python-org-web-forum>`_
    • triaging/labels.rst:413:.. _release schedule: https://devguide.python.org/#status-of-python-branches
  • Also, I'm still curious about the under/over header format used here—while an extra column on each side is included in some of the docutils examples, it is not what is specified in either the Sphinx documentation or the canonical Python docs style guide in this very repo, nor have I commonly seen it elsewhere...

internals/garbage-collector.rst Show resolved Hide resolved
documenting/translating.rst Outdated Show resolved Hide resolved
triaging/labels.rst Outdated Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
developer-workflow/communication-channels.rst Show resolved Hide resolved
advanced-tools/clang.rst Outdated Show resolved Hide resolved
advanced-tools/clang.rst Outdated Show resolved Hide resolved
advanced-tools/index.rst Outdated Show resolved Hide resolved
@@ -1,70 +0,0 @@
Appendix: Topics
Copy link
Member

Choose a reason for hiding this comment

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

Is this file completely gone?
The file itself is probably redundant after this PR, but are the "Recommended readings" preserved somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

are the "Recommended readings" preserved somewhere?

No. Where would you suggest, if we should keep them?

A

Copy link
Member

Choose a reason for hiding this comment

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

As @ezio-melotti suggests right below, they could go under the Audience categories (Contributor, Triager, Documentarian, CoreDev) in the restored table in the index. That would be a lot better than reverting whole removal and is the same or less work than reverting the removal since it would mean updating the doc links to ref links and fixing the target names, or fixing the outdated file paths and names, and also adding the pages that were added/split and now missing here or in the wrong sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

updating the doc links to ref links and fixing the target names, or fixing the outdated file paths and names, and also adding the pages that were added/split and now missing here or in the wrong sections.

I did these in a commit that I kept, so they should still work. Time-wise I probably don't have time to go through and update the table in the next week or so, so reverting was the quickest option.

A

Copy link
Member

@CAM-Gerlach CAM-Gerlach Jun 19, 2022

Choose a reason for hiding this comment

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

For the table (as far as I can tell), but not the appendix, at least as of the version you've pushed here—it still uses :doc: links, and almost all of them are now broken, as can be seen on the preview. Furthermore, it is missing the documents split as part of this PR, leaving it incomplete.

Instead of tediously updating all of that (or introducing a major regression), only to remove it again in a followup PR, I suggest just spending the same or less time re-dleeting it and adding them to the table instead as nessesary. Additionally, to keep it shorter and less work to update, we can just link the top-level index pages of sections that are obviously of central relevance to the audience type in their entirety, instead of linking every sub-page.

Here's what I suggest as the layout, and I'm happy to make a PR to your branch to implement it if you don't have time (which should take around the same or less as fixing the appendix):

Suggested table structure (by column heading)

All contributors:

  • Getting Started
  • Following Python's Development
  • Issue Tracker
  • Running and Writing Tests

Documentarians:

  • Documentation
  • Development Cycle
  • Experts Index
  • Triaging an Issue

Triagers:

  • Issues and Triaging
  • Development Workflow
  • Experts Index
  • Accepting Pull Requests

Core Developers:

  • Core Developers
  • Development Workflow
  • Issues and Triaging
  • Testing and Buildbots

core-developers/developers.csv Outdated Show resolved Hide resolved
core-developers/developer-log.rst Show resolved Hide resolved
index.rst Show resolved Hide resolved
index.rst Show resolved Hide resolved
internals/garbage-collector.rst Show resolved Hide resolved
internals/garbage-collector.rst Show resolved Hide resolved
triaging/triage-team.rst Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member Author

I do have a concern on scope -- for instance title case / sentence case of headings. I would suggest to merge-as-is (once I've resolved the outstanding points) and do those sort of things as follow-ups.

A

@AA-Turner
Copy link
Member Author

Fixed references, increased max depth, removed spaces from titles, reverted line ending change in developers.csv.

A

@AA-Turner
Copy link
Member Author

I restored the appendix and topic table again to reduce scope of this PR as it seems removing them needs more discussion (Ezio's points).

Partially I want to get this in as it puts this / other PRs in a state of limbo w.r.t. the moves.

A

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Minor note: The three remaining instances of the devguide links using external URLs were updated in the text, but the link targets were not actually removed. Searching for \.\. _\w+.*: .*devguide\.python should find all of them.

While I agree we shouldn't scope-creep this further and change the style of all the headings, I don't think we should rush this PR now without resolving the final issue with the appendix and index table being redundant/out of date, since:

  • These are pretty central to the question of how to organize and structure the devguide (more so than some of the other implemented changes)
  • This would result in a significant regression since the appendix is missing the newly split docs, has out of date file paths and is either out of sync with or duplicate much of the new structure, unless we do the work here to update it.
  • There seems to be a fairly straightforward solution suggested by multiple people here, which would probably be around the same amount of work as updating the existing redundant appendix.

Also, keep in mind this can't be merged immediately until someone with the appropriate RTD permissions sets up the redirects (or we do them ourselves with my script or sphinx-redirects), and a repo admin can merge the PR (due to the CLA bot bug)— @ezio-melotti are you one here? And the existing PRs should transfer, so long as the move was detected (as most were) instead of the page split, and the relevant text wasn't changed.

As @ezio-melotti suggests, the various recommended reading relevant to each of the audience types can go under the respective table colums (Contributor, Triager, Documentarian, CoreDev) in the table on the index page index. That would be a lot better than reverting whole appendix removal and should be a similar amount of work as updating both to properly reflect the changes.

.. _GSoC: https://summerofcode.withgoogle.com/
.. _issue tracker: https://devguide.python.org/tracker/
.. _GitHub pull requests: https://github.com/python/cpython/pulls
.. _Python source code repositories: https://github.com/python/cpython/
.. _Reporting security issues in Python: https://www.python.org/dev/security/
.. _python-ideas: https://mail.python.org/mailman3/lists/python-ideas.python.org
.. _release schedule: https://devguide.python.org/#status-of-python-branches
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.. _release schedule: https://devguide.python.org/#status-of-python-branches

This was fixed above and marked resolved but missed being removed here.

@hugovk
Copy link
Member

hugovk commented Jun 20, 2022

Also, keep in mind this can't be merged immediately until someone with the appropriate RTD permissions sets up the redirects (or we do them ourselves with my script or sphinx-redirects), and a repo admin can merge the PR (due to the CLA bot bug)— @ezio-melotti are you one here?

Some smallish benefits to having Sphinx-level redirects (either your script, sphinx-redirects, sphinx-reredirects, sphinxext-rediraffe, ...) in addition to more-efficient RTD ones:

  • Redirects go live immediately, don't need to sync with setting RTD config
  • We get redirects on our forks on RTD (without having to also set up the RTD config)
  • We get redirects for local testing
  • They're defined in code (although it would a good to put the RTD config under source control for transparency, copying and pasting)

@ezio-melotti
Copy link
Member

a repo admin can merge the PR (due to the CLA bot bug)— @ezio-melotti are you one here?

Apparently not, as I don't have the option to force-merge, nor access to the repo settings (I requested access in python/steering-council#128).

@CAM-Gerlach
Copy link
Member

I definitely agree; having them in the repo and generating them as part of the build process allows testing them locally, is portable between hosts and deployment mechanisms, and makes 100% sure they are in sync with the repo itself (particularly in this case, since otherwise a bunch of inbound links will break), among the other benefits you mentioned.

sphinxext-rediraffe seems really powerful and well suited to this use case, as it can resolve chained redirects while only actually doing a single redirect client-side, can check that deleted/moved pages in the GitHub history are directed, and is an integrated Sphinx extension. Sphinx-reredirect seems to be the other option; its simpler and a bit fresher in terms of maintenance, but doesn't seem to be nearly as sophisticated as rediraffe, especially with its handy features well suited for this use case.

Of course, the RTD config should also be checked in to the repo, especially on one with many contributions/contributors and few admins with limited bandwidth; I've found in general that always checking in infra config and updating them in sync with PRs is far more maintainable than buried in the config of some external service asynchronous from the content changes and that only a few people have access to.

@AA-Turner AA-Turner closed this Jun 20, 2022
@AA-Turner AA-Turner reopened this Jun 20, 2022
@hugovk
Copy link
Member

hugovk commented Jun 20, 2022

Good news, the CLA bot is fixed! We can do a regular merge when we're ready!

@pradyunsg
Copy link
Member

Here's an example of how to have HTML meta redirects without adding a new extension or such, within regular reStructuredText.

https://raw.githubusercontent.com/pypa/pip/main/docs/html/reference/pip_cache.rst

@pradyunsg
Copy link
Member

pradyunsg commented Jun 24, 2022

Given that all of the newly introduced "root" documents are basically just toctrees, I think an option worth considering is:

  1. Not moving the pages around, and only changing the organisation in the Sphinx toctree directives.
  2. Having multiple toctrees with a caption on each, for grouping the various documents -- Furo's own documentation has a toctree with a Development caption: https://raw.githubusercontent.com/pradyunsg/furo/main/docs/index.md

This would bring the navigation closer to something similar to https://tailwindcss.com/docs/.

I haven't looked at the actual code changes, only the rendered documentation, but IMO this approach would also lend toward an easier-to-review PR -- since none of the files would move, none of the pages need updates to their references and the markup changes/cleanups can move into a dedicated follow up. It's also more incremental and likely slower, but it will make reviewing the code changes easier and also result in shorter PRs (which tend to not have as many things slip through the cracks as large reorganisation PRs). :)

@AA-Turner
Copy link
Member Author

Done.

A

@ezio-melotti ezio-melotti merged commit 355d151 into python:main Jul 11, 2022
@ezio-melotti
Copy link
Member

Merged, thanks @AA-Turner for the PR and to all the people who reviewed it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants