I’m a new contributor and was surprised to see that there’s no pull request template for opening a PR against Django.
I think a PR template can be really helpful to know what one should put in a PR description, especially for new contributors who are just getting started. It can feel a bit daunting for a first-time contributor to have to write the PR description from scratch. I think that even a very simple template could be helpful (for exmple django-cms’s pull request template is very simple).
I’m keen to hear other people’s opinions on this
Hello @erosselli! Thank you for this proposal.
I agree that this is a necessary improvement for enhancing the PR creation experience. I would certainly like to include in the checklist a reminder for contributors to keep the Trac ticket updated, as this is one of the areas where I often notice a lack. Then of course certainly list tests and docs as necessary too. +1 from me!
As I have posted previously, I think the PR process (beyond creation) could also benefit from reminders and/or automatic messages prompting authors to take the necessary steps to get their branch in the ‘needs review’ list, so I’ll follow up about this as well.
As someone who contributes very infrequently, having reminders would be helpful.
Hellooooooo I personally like PR templates.
We already have a welcome message for new contributors which points out a few useful resources and a patch review checklist in the contributor docs. We may need to think if we want to duplicate this information or to migrate it as a new source.
The main things for me are:
- the title format of the PR which results in the auto linking to Trac
- updating the flags on the PR when you want a new review and pointing out the review queue
- do not commit any unnecessary changes (such as reformatting)
- bugs and new features needs tests
- new features or changes to features need a doc and release note update
- be patient, avoid @-ing people. A note that the reviewers are volunteers and that the fellows have a lot on their plate. Having weeks or months between reviews is not unheard of and please be really kind.
But I think it would be nice to have information in a collapsed section so that reviewers are not distracted by helper information.
Side note, I tried to start a discussion on this on the mailing list but it didn’t get much attention: https://groups.google.com/g/django-developers/c/rl-t4BMcJcQ/m/VBe9dy3oBAAJ
I much prefer the forum
Thanks for the suggestion, however, in most (all) cases contributors simple delete their content and do whatever they want. Also, we already have a welcome message for the first time contributors which contains remainders:
As it’s your first contribution be sure to check out the patch review checklist.
If you’re fixing a ticket from Trac make sure to set the “Has patch” flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
And it doesn’t change anything, folks don’t check the patch review checklist, they don’t mark flags in the tickets, etc. I don’t believe that adding PR template will change anything.
My 2 cents.
Yes to all of this. Also, I would ask to please please pretty please avoid using the “patch” terminology since I think that only made sense when Django received literally patch files to fix issues/bugs. Now that we use pull requests, I think we should stick to that terminology because it’d feel more familiar/known to new contributors.
It’s now possible to define forms rather than just a text-based template. (See, for example, what has been done for the CPython repo.)
This may have more success, but anything is still subject to people skipping sections or putting in rubbish values.
I’d be happy to see a trial of something here, especially as the form approach would be more guided and up-front, whereas the new contributor message only gives guidance after the PR has already been submitted.
I’m a fan of pull request templates, and it was a huge win for several projects I contribute to, including Awesome Django.
While I think referencing an external document is fine, having the requirements you want in the PR template spelled out so people can work with it is helpful and less overwhelming.
I’d also keep in mind that what’s normal for Django isn’t always what’s normal for other projects and I see a ton of projects that use pull request templates.
+1 from me for a minimal template. A few choice links to the review checklist would be good. Yes, people can easily ignore them, but they’re less easy to ignore than a link to a separate document.
Also, +1 for changing “patch” to “pull request” in Django’s terminology, including the review checklist in the docs.
Forms are for issues only, right? It would be nice to have them on PRs but I don’t think that’s possible yet.
I think this is a bit unfair and an oversimplification. I read the templates (when available) carefully and I know of many others that do as well.
From my view, the template and the first time contributors are complementary. The advantages of the template are:
- Is there every time you open a PR, not only on your first contribution. What happens if you contributed 3 or 6 months ago? It’s very easy to forget what’s needed for this project in particular. Django has a non trivial process for proposing PRs (for good reasons, but still) and it’s very easy to miss things. This is exactly what @CodenameTim said.
- For all those making a first PR with “make toast”, which are closed immediately, the first time contributor message gets completely lost in the “learning how to do a PR process”.
- First time contributor messages are more widely use as welcoming messages and introducing concepts for the first time, not so much about reminding the author about things that need to happen on every single PR.
I sometimes forget details about the commit message format and setting flags in Trac. If it happens to me, I’m sure others may also encounter similar challenges (though I know you are exceptionally well-versed in these matters).
While I acknowledge the skepticism, I believe even a small change would be a good move. And what if it works because it’s a method more people are likely familiar with? Considering the minimal cost for us and the potential for a positive return higher than zero, I believe it’s a win! There’s also the possibility of it turning out to be a medium to large win, which would be a pleasant surprise
Glad to see you on the forum @erosselli just wanted to say it’s really cool we have people coming in with fresh eyes and proposing simple improvements like this
I think that whatever we define as a PR template is better than not having a template at all. An example of a very simple template I think could be useful would be something like:
Briefly describe the problem or reason for the changes implemented in the PR
Provide link for the Trac ticket (if applicable)
Briefly describe any tests you have added or updated to test the changes introduced by the PR
I have updated the Trac ticket’s flags (set
Has Patch flag)
I have added or updated tests to test the changes I made
I have added or updated relevant documentation (if applicable)
I have gone through the Patch Review Checklist
I’d be happy to submit a PR if this is something people think would benefit the community, and to make any changes to this example template (or use a different example template altogether)
Thank you @erosselli! Below some suggestions for the template, I would say yes, create a PR and we can iterate there.
This should really say:
Link to the accepted Trac ticket for this PR. All PRs except docs or tests changes need an accepted ticket. New features need community consensus.
Then, I would not use the “patch” terminology, we should say branch or pull request. Lastly, I think we should also add:
- Guidelines for commit message (past tense, mentions the ticket #, ends with a dot)
- PRs need to target the
main branch. Backport to other stable branches will be done by mergers when necessary.
- A “More details” link at the bottom pointing to:
I have pushed a draft PR with the proposed PR template. Any feedback or comments are welcome!