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

Add notes on runtime version access #1607

Closed
wants to merge 15 commits into from

Conversation

ncoghlan
Copy link
Member

@ncoghlan ncoghlan commented Oct 6, 2024

Also redirects the obsolete single-source version guide to the updated single-source version discussion.


📚 Documentation preview 📚: https://python-packaging-user-guide--1607.org.readthedocs.build/en/1607/

Also redirects the obsolete single-source version guide to the
updated single-source version discussion.
@ncoghlan
Copy link
Member Author

ncoghlan commented Oct 6, 2024

Seeing if we can make HTTP redirects work without an extension like https://pypi.org/project/sphinx-reredirects/

Test link for the redirect: https://python-packaging-user-guide--1607.org.readthedocs.build/en/1607/guides/single-sourcing-package-version/

Edit: the redirect is triggering, but the current relative URL isn't right. Will investigate further later this evening.
Edit 2: I was misinterpreting a slow rendering of the RTD 404 page as the redirect triggering. Instead, my test link just had the old URL wrong. Fixed now (and the old guide link immediately redirects to the new discussion page)

@ncoghlan
Copy link
Member Author

ncoghlan commented Oct 6, 2024

Found a surprising CPython docs issue here: python/cpython#125018

@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2024

@ncoghlan thanks for taking this on! I was hoping to take a stub at actually writing some content, but it doesn't look like I'm going to get to that anytime soon, so I guess this is the way.

Seeing that you're repeating some of #1276, I think it would be nice to add a Co-Authored-By: wim glenn <hey@wimglenn.com> trailer to Git commits… That's possible via git commit --amend --trailer='Co-Authored-By: wim glenn <hey@wimglenn.com>'. WDYT?

wim glenn <hey@wimglenn.com>:
@ncoghlan ncoghlan self-assigned this Oct 6, 2024
Copy link
Contributor

@ChrisBarker-NOAA ChrisBarker-NOAA left a comment

Choose a reason for hiding this comment

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

Thanks for finishing this up!

source/discussions/versioning.rst Outdated Show resolved Hide resolved
source/discussions/single-source-version.rst Outdated Show resolved Hide resolved
@ncoghlan
Copy link
Member Author

ncoghlan commented Oct 6, 2024

I posted a PR for the lack of semantic reference targets in the CPython docs, so assuming that review goes smoothly I may be able to fix this PR to use proper references before it gets merged: python/cpython#125027

@webknjaz
Copy link
Member

webknjaz commented Oct 6, 2024

@ncoghlan so I made a mistake in the suggested attribution command earlier (I've updated that comment to correct it since) and missed a chunk of the trailer in it, which is why GitHub didn't attribute that commit to wim glenn. It was supposed to be git commit --amend --trailer='Co-Authored-By: wim glenn <hey@wimglenn.com>'.
I must note, however, that this command modifies the last commit, and you seem to have produced a separate empty commit (I'm guessing with --allow-empty).

Normally, I'd suggest just putting this trailer as a Co-Authored-By: wim glenn <hey@wimglenn.com> line into the merge commit and use the squash button. But this repository is set up to use natural merge when accepting PRs, so you won't have this option. All of the individual commits here will become a part of main and the empty commit will be what's credited. So my recommendation is to squash the commits locally, with an interactive rebase, adding that line to the resulting commit message. This can be done right before merging.

ncoghlan and others added 2 commits October 7, 2024 16:34
Co-authored-by: Éric <merwok@netwok.org>
* address review comments
* go back to semantic cross-references (the build will fail until the
  CPython PR and its backports land, but that's OK)
Copy link
Member Author

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

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

Assorted review comments addressed. Build will currently fail, as I went back to using semantic cross-references to importlib.metadata APIs (in anticipation of getting the stdlib docs update PR and its backports merged)

source/discussions/versioning.rst Outdated Show resolved Hide resolved
source/discussions/versioning.rst Outdated Show resolved Hide resolved
source/discussions/single-source-version.rst Outdated Show resolved Hide resolved
@ncoghlan
Copy link
Member Author

ncoghlan commented Oct 7, 2024

@webknjaz Huh, interesting, I hadn't realised that squash merges could be disabled outright (although I can see that enabling the merge queue would do so). I'll do the full squash on the PR branch just before merge (since it messes up the comment threads if I do it beforehand)

@webknjaz
Copy link
Member

webknjaz commented Oct 7, 2024

@ncoghlan yep, you're right. Technically that's not disabled directly but when merge queues is required for a branch, it only lets one select a single method. Overriding it is possible for admins as they'd be able to bypass the merge queues when force-merging.

@ncoghlan
Copy link
Member Author

ncoghlan commented Oct 8, 2024

The semantic link targets are live on docs.python.org, so I'll remove the ! prefixes from the PR (since https://docs.python.org/3/library/importlib.metadata.html#importlib.metadata.version is a valid target now).

source/discussions/single-source-version.rst Outdated Show resolved Hide resolved
source/discussions/versioning.rst Outdated Show resolved Hide resolved
source/discussions/versioning.rst Outdated Show resolved Hide resolved
tabedzki

This comment was marked as duplicate.

@ncoghlan
Copy link
Member Author

Given the +1's above and the lack of objections on the discuss.python.org thread, I'm going to go ahead and merge this one to give us a solid foundation for any future updates.

@ncoghlan
Copy link
Member Author

@webknjaz Rather than force-pushing to this branch, I decided it would be cleaner to supersede this PR with a new one (so the comment history doesn't get messed up), but that does mean the new PR needs approval from a second reviewer: #1612

@ncoghlan ncoghlan closed this Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants