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

Change links to label refs #98454

Merged
merged 5 commits into from Oct 26, 2022
Merged

Conversation

slateny
Copy link
Contributor

@slateny slateny commented Oct 19, 2022

No description provided.

@slateny slateny requested a review from ericvsmith as a code owner Oct 19, 2022
@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Oct 19, 2022
@@ -461,7 +461,7 @@ Module State Access from Slot Methods, Getters and Setters

.. After adding to limited API:
Copy link
Contributor Author

@slateny slateny Oct 19, 2022

Choose a reason for hiding this comment

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

This part doesn't render (see page) so unsure what this should look like.

Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 19, 2022

Choose a reason for hiding this comment

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

It's a reST "comment", which doesn't render; not sure if its intended to be or not, since it does use reST syntax within itself.

@encukou any insight here?

@slateny slateny requested a review from CAM-Gerlach Oct 19, 2022
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Thanks @slateny ; a couple comments.

At least some of these are likely because things were converted from PEPs, and should be less common now that we added Intersphinx support there.

@@ -461,7 +461,7 @@ Module State Access from Slot Methods, Getters and Setters

.. After adding to limited API:
Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 19, 2022

Choose a reason for hiding this comment

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

It's a reST "comment", which doesn't render; not sure if its intended to be or not, since it does use reST syntax within itself.

@encukou any insight here?

Doc/reference/datamodel.rst Outdated Show resolved Hide resolved
Doc/library/dataclasses.rst Outdated Show resolved Hide resolved
slateny and others added 2 commits Oct 20, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@slateny
Copy link
Contributor Author

slateny commented Oct 20, 2022

Can't reach it with inline comments, but there are also links like here (linking to 3.4 whatsnew changelog) that I'm not too sure if it can be converted into a label. With a changelog label, such as in the 3.7 whatsnew, if not on the right version on the webpage (.org/3/ instead of .org/3.7), the changelog link goes to the most current one (3.10 ish), while in the hardlinked whatsnew, such as the 3.6 whatsnew, changelog goes to the right page.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Oct 20, 2022

With a changelog label, such as in the 3.7 whatsnew, if not on the right version on the webpage (.org/3/ instead of .org/3.7), the changelog link goes to the most current one (3.10 ish), while in the hardlinked whatsnew, such as the 3.6 whatsnew, changelog goes to the right page.

Instead of linking to a whole different docs build, what should instead be done is to tweak blurb here to add standard ref target labels for each section (and subsection) when generating the news file based on the version tag, e.g. 3.11.0-alpha-1 or 3.11.0 for the final. Then, the appropriate sections for each version, etc., can be linked. However, that would be best addressed in a separate PR after the change to blurb is made and deployed, assuming it is accepted.

There are a few remaining relevant instances that can be fixed in this PR:

Doc/using/venv-create.inc:20:   <https://docs.python.org/dev/whatsnew/3.6.html#id8>`_.
Doc/whatsnew/2.2.rst:27:Reference <https://docs.python.org/2.2/lib/lib.html>`_ and the `Python
Doc/whatsnew/2.2.rst:28:Reference Manual <https://docs.python.org/2.2/ref/ref.html>`_.  If you want to
Doc/whatsnew/2.2.rst:398:https://docs.python.org/dev/howto/descriptor.html is a lengthy tutorial introduction to
Misc/NEWS.d/3.9.0a3.rst:809:<https://docs.python.org/3/library/inspect.html#types-and-members>`_

At some point, if we add pre-commit checks to this repo, we could add a check for these links, like I added for hardcoded links to PEPs and RFCs on the PEPs repo (which there also are a handful of here, BTW, and could be fixed in a separate PR).

@slateny slateny requested a review from vsajip as a code owner Oct 21, 2022
@slateny
Copy link
Contributor Author

slateny commented Oct 21, 2022

I left out

Doc/whatsnew/2.2.rst:27:Reference <https://docs.python.org/2.2/lib/lib.html>`_ and the `Python
Doc/whatsnew/2.2.rst:28:Reference Manual <https://docs.python.org/2.2/ref/ref.html>`_.  If you want to

for now as the pages are far too different to the corresponding modern links, and uncertain if those should be changed over or if they should just remain for historical purposes (as those pages seem to remain for historical purposes too).

@CAM-Gerlach CAM-Gerlach requested review from JelleZijlstra and removed request for vsajip and ericvsmith Oct 21, 2022
@JelleZijlstra
Copy link
Member

JelleZijlstra commented Oct 21, 2022

Yes, let's leave the old what's news unchanged as much as possible.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

LGTM, thanks @slateny

@@ -2052,6 +2052,8 @@ tkinter
The :mod:`tkinter.tix` module is now deprecated. :mod:`tkinter` users
should use :mod:`tkinter.ttk` instead.

.. _whatsnew-3.6-venv:
Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 21, 2022

Choose a reason for hiding this comment

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

Sidenote: Following the convention I've used for the 3.11 What's New would be whatsnew36-venv, but I think this convention (or at least whatsnew3.6-venv) would be better—out of old habit I often avoid using . in ref target labels. In any case, no practical reason to change it here.

Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 21, 2022

Choose a reason for hiding this comment

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

Sorry for the confusion—I was mentioning this as a sidenote and suggestion you not change it here (instead, I really should be using it in the ref targets I'm adding, though probably too late now for 3.11—I can adopt it with 3.12). But since you have changed it, not really worth changing it again.

Just to be more clear for the future, if I'm actually requesting a change, I will pretty much always make it as a actual suggestion (unless its impractical or impossible), so you can easily apply it with Files -> Add to batch -> Commit rather than manually implementing, committing, pushing and resolving it. The way I see it is if I'm going to ask someone else to make a change on their PR (unless its really important), its only fair that I put in the work myself to implement it, so all they have to do is click a button to accept it.

Copy link
Contributor Author

@slateny slateny Oct 22, 2022

Choose a reason for hiding this comment

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

I see I see, don't worry too much about it - I checked the existing labels and just decided that it would be good to follow convention. And conversely, I requested the review so I definitely don't mind the work (and I would have to run it locally anyway just to check the rendering), but of course the ease of changing is always appreciated.

@@ -805,8 +805,7 @@ event loop only if called from the main thread.
.. section: Documentation

Add an entry for ``__module__`` in the "function" & "method" sections of the
`inspect docs types and members table
<https://docs.python.org/3/library/inspect.html#types-and-members>`_
:ref:`inspect docs types and members table <inspect-types>`.
Copy link
Member

@CAM-Gerlach CAM-Gerlach Oct 21, 2022

Choose a reason for hiding this comment

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

Seems a bit cleaner to do

Suggested change
:ref:`inspect docs types and members table <inspect-types>`.
:mod:`inspect` docs :ref:`inspect-types` table.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Oct 21, 2022

I left out for now as the pages are far too different to the corresponding modern links, and uncertain if those should be changed over or if they should just remain for historical purposes (as those pages seem to remain for historical purposes too).

Yeah, not much point changing those

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

LGTM, thanks @slateny

@JelleZijlstra JelleZijlstra merged commit 268129a into python:main Oct 26, 2022
14 checks passed
@miss-islington
Copy link
Contributor

miss-islington commented Oct 26, 2022

Thanks @slateny for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒🤖

@miss-islington
Copy link
Contributor

miss-islington commented Oct 26, 2022

Sorry, @slateny and @JelleZijlstra, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 268129a74f01adb7bb14cd71d1f38378e39d304d 3.11

@miss-islington
Copy link
Contributor

miss-islington commented Oct 26, 2022

Sorry @slateny and @JelleZijlstra, I had trouble checking out the 3.10 backport branch.
Please retry by removing and re-adding the "needs backport to 3.10" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 268129a74f01adb7bb14cd71d1f38378e39d304d 3.10

@JelleZijlstra
Copy link
Member

JelleZijlstra commented Oct 26, 2022

@slateny would you be able to do the backports?

@slateny
Copy link
Contributor Author

slateny commented Oct 26, 2022

Yep sure

@slateny slateny deleted the s/remove-hard-links branch Oct 26, 2022
slateny added a commit to slateny/cpython that referenced this pull request Oct 26, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
(cherry picked from commit 268129a)
@bedevere-bot
Copy link

bedevere-bot commented Oct 26, 2022

GH-98725 is a backport of this pull request to the 3.11 branch.

slateny added a commit to slateny/cpython that referenced this pull request Oct 26, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
slateny added a commit to slateny/cpython that referenced this pull request Oct 26, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-bot
Copy link

bedevere-bot commented Oct 26, 2022

GH-98726 is a backport of this pull request to the 3.10 branch.

JelleZijlstra pushed a commit that referenced this pull request Oct 26, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
(cherry picked from commit 268129a)
JelleZijlstra pushed a commit that referenced this pull request Oct 26, 2022
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants