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

Improvements to Lifecycle of a pull request #1358

Open
ziima opened this issue Jul 19, 2024 · 3 comments
Open

Improvements to Lifecycle of a pull request #1358

ziima opened this issue Jul 19, 2024 · 3 comments

Comments

@ziima
Copy link

ziima commented Jul 19, 2024

I write this issue as a feedback from Euro Python 2024 sprint to denote troubles I encountered as I contributed to cpython.

Describe the enhancement or feature you'd like
Missing content in "Lifecycle of a pull request" https://devguide.python.org/getting-started/pull-request-lifecycle/

  • "Quick guide" is missing a step to create NEWS.d record. It's just very briefly mentioned in patchcheck.
  • "Step-by-step guide" doesn't denote a branch naming convention. I was told to use gh-<number>-<name>.
  • "Making good commits" doesn't mention gh-<number> should be included in the commit message.
  • "Step-by-step guide" doesn't mention gh-<number> should be included in the PR title.

Describe alternatives you've considered
None.

@picnixz
Copy link
Contributor

picnixz commented Jul 20, 2024

My 2 cents: I don't like having gh-<number> in the commit message or in the branch because the issue logs become extremely noisy due to GitHub saying something like "XXX added a commit that referenced that issue YYY hours ago". Not sure if it's my GH extension though but I think the PR title should be sufficient in general.

However, the first and last points are worth mentioning (because otherwise the CI/CD is really unhappy).

Some precision: while I understand the "making good commits" section, this is usually fine if you only have a single commit or and likely don't modify more things afterwards. After a review, it is common to update typos or address reviews and I honestly don't think that adding gh-<number>: address review would give you a better commit message. What's important IMO is that you organize your commits in such a way that they are incrementally easy to review (I say this but my commits also fail to follow this rule) and don't force-push unless you forgot to locally squash multiple commits into one (like, two commits for fixing up a typo). But my experience tells me that you can easily have some surprises if you always rebase+force-push :D

For the branch, I think it's essentially for you to remember which branch you are working on. My local branches are sometimes named differently than my remote branches just for triaging them (I mean, if you have many "fix-bug-in-this-module" branches you don't necessarily remember which issue you are working on...)

@JelleZijlstra
Copy link
Member

As a core developer merging PRs, I don't really care what you name your branch; it doesn't affect me.

The main way commit messages matter is that they affect the final commit message that will get used when I merge the PR. GitHub gives a text box pre-filled with the existing commits. I usually just remove all but the first one (since they're mostly going to be small fixes and cleanups), so it doesn't really matter to me what you put there.

@erlend-aasland
Copy link
Contributor

We don't need a branch naming convention, since we do not create branches on the python/cpython repo; they should always be created on the developers fork, so you can chose whatever convention you like.

This information could be included in the devguide using a better wording.

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

No branches or pull requests

4 participants