-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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-94635: Frame sqlite3 how-to headings as such & move default adapters to Reference #96136
gh-94635: Frame sqlite3 how-to headings as such & move default adapters to Reference #96136
Conversation
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with some nits. I suggest to leave the adapter paragraphs as they are for this PR; we can discuss how to handle them after landing this.
(I took the liberty to sync with main; hope you don't mind.)
Per project/org-wide policy in the Spyder org and other projects I'm involved with (and personally as well), we strongly prefer rebases to merges to update the base branch. However, a big part of that is the fact that those projects historically always used merges rather than squashes for pull requests, though in recognition of the inevitably of many contributors having poor commit hygiene and the cost-benefit of dealing with it otherwise, we've begun to use more squash-merging in those cases instead (thanks largely due to my own advocacy and experiences with squashing on Python-org projects, despite being one of the biggest advocates for proper commit hygiene and pre-merge rebase cleanup on PRs). Merges instead of rebases for updating the base branch can create huge problems for authors, reviewers and committers in that workflow, which are much less of a concern when squashing. Still, it makes navigating commit diffs, blame and history locally on the command line, even with Also, when I do have to merge for some reason, I always make sure to give the merge commit an actually useful commit message rather than a rather unhelpful statement simply that there was a merge, without any meaningful information about why it was done or what it updated. |
For CPython, |
Yes, there is. I'll find it when I'm back at my computer. |
Why
I'm not totally sure what's being referred to here beyond the natural consequences of rebases (e.g. not seeing outdated check runs), at least as a Refined GitHub user. Might you be able to be more specific (out of curiosity), if it isn't too much trouble?
It would also be nice to know any tricks for making |
Because it creates a merge commit, which |
Not for me, even with Refined GitHub (and certainly not without it before)—is there some setting I'm missing?
Oh how so? I'm not sure what this looks like, exactly. |
Sure; with rebases, you'll lose older commits, which imply losing track of earlier workflow runs, and also losing track of earlier review comments and discussions, both very frustrating inconveniences specifically when it comes to reviewing code changes. This is why a lot of core devs will frown upon and nitpick about rebases. There is one exception though: if one opens a draft PR, it is ok to rebase the PR until it is marked as ready for review. Once that is done, one should treat the PR as being under review, so further syncs with main should be done using |
# List commits like the GitHub UI does
$ git log --oneline --first-parent
# Only list merge commits (syncs with main); note that this is not possible if you rebase
$ git log --oneline --merges To list the commits in this PR like the GitHub UI does, do: $ git log --oneline --first-parent 8dc623169e4db60e5e92d816e9a16290ff5306bb~..HEAD
6aa271cc5c (HEAD -> sqlite3-doc-cleanup-final-headings) Merge branch 'main' into sqlite3-doc-cleanup-final-headings
5eb1abca3e Reframe How-To section headers in terms of how-to per Diataxis
8dc623169e sqlite3 doc: Move default adapters and converters to Reference section
Ah, I was a little bit unclear; it is not the merge commit that is hidden, it is the commits to
The effect of this is that this PR now consists of three commits, two "regular" commits, and a merge commit. You can confirm this visually by clicking the commits tab in the UI for this PR. The merge commit makes sure I can still easily inspect the previous workflow runs via the PR UI; a rebase would have removed this possibility. You can inspect commits, for example using
Thinking about it, a |
Say, if I'd created three new commits on-top of the merge commit, I'd simply do A better solution may be to find the nearest merge commit and rebase onto that. For example: $ git log --oneline --merges -n 1
6aa271cc5c Merge branch 'main' into sqlite3-doc-cleanup-final-headings
$ git rebase -i 6aa271cc5c
Successfully rebased and updated refs/heads/sqlite3-doc-cleanup-final-headings.
$ For convenience, you can then alias those operations so you'd have shortcuts for show-me-the-last-merge and rebase-onto-the-last-merge. |
Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
Yeah, mentioned that one previously, though it varies by PR how many outdated runs are lost track of here (often only one, which is then re-run on the same rebased commit), and how useful they really are. As a reviewer and maintainer, there are a few specific cases where this can be inconvenient, but at least in my experience it is relatively uncommon that I actually actually need this—but maybe its more common for CPython, I imagine on non-docs PRs.
Oh, really? If so that's really not good, but I've always kept a careful eye on this and I don't believe I've seen ever review comments actually disappear after rebases. They can be marked Outdated, but as far as I've been able to tell in my impromptu testing, at least nowadays that is based only on whether the content of the line(s) in question actually changed (which should be the same whether there is a merge or a rebase).
Ah,
Oh, you were just referring to that (i.e. what I was looking for a local equivalent of, i.e.
Ah yeah, "fast forwarding" after both
Oh, I didn't meant rebasing after the merge which is essentially trivial; what I was referring to was rebases (fixup, reorder, etc) to the commits before it, or mixing both, which is something we had to do quite a bit for various reasons to PR branches (copyright, changes to binary files, bad commits, poor commit hygiene, etc) when not using squash merges. But as mentioned, that's not really relevant for CPython which does (and on which I'd normally avoid any non-update rebasing and make any changes as separate commits to avoid any problems for reviewers, of course), so if you do have any ideas on that (beyond just cherry-picking and/or basically undoing the merge commit and doing a clean rebase instead) feel free to send them over Discord—I've wandered far enough off topic as it is. And most of that is more easily avoided by just doing a squash merge, which is what we tend to do more these days. |
Not exactly; a rebase onto a new base always creates new commits (aka rewrites history). A fast-forward merge does not rewrite history, nor does a non-fast-forward merge. If you try to do a
Aha. No, that's always painful. |
No problem; I enjoy nerding about git :) |
Sorry for all the git noise, Ezio! I'm ready to land this, but since CAM requested a review from you, I'll hold it until you've given your thumbs up. Thanks, both of you 🙏🏻 |
Sorry as well, mostly my fault rather than Erlend's, heh. Approve/merge at your discretion; I applied the first two suggestions and a somewhat different one for the third, but also wanted to give you another chance to chime in.
Sorry, I wasn't clear—what I meant was if there was such a thing as a "fast forward" when both branches have additional commits (i.e. a pull request updating the base branch), it would be functionally equivalent to a rebase, not a merge—which is what you are saying as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @CAM-Gerlach for the PR, and @erlend-aasland for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11. |
Sorry, @CAM-Gerlach and @erlend-aasland, I could not cleanly backport this to |
Sorry @CAM-Gerlach and @erlend-aasland, I had trouble checking out the |
…efault adapters to reference (pythonGH-96136) Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com> Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>. (cherry picked from commit 6bda5b8) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
GH-96226 is a backport of this pull request to the 3.11 branch. |
…efault adapters to reference (pythonGH-96136) Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com> Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>. (cherry picked from commit 6bda5b8) Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
GH-96227 is a backport of this pull request to the 3.10 branch. |
…adapters to reference (python#96136) Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com> Co-authored-by: Ezio Melotti <ezio.melotti@gmail.com>
As discussed in #94635 and heavily inspired by #94677 , rename the
sqlite3
library documentation's "How-to" section headings in the form of "How-to ..." per Diataxis, and also move theDefault Adapters and Converters
section to the Reference, where it better belongs (as it simply describe the module's contents/behavior rather than stating how to do anything).I wasn't sure what to do about "Adapter and Converter Recipes", so I've left it along for now. Any ideas?