Simplify pull request template #13916
Conversation
.github/pull_request_template.md
Outdated
| ( ) Documentation PR is available with #prnumber. | ||
| ( ) Documentation issue #issuenumber is filed, and can be handled later. | ||
|
|
||
| ## Release notes |
There was a problem hiding this comment.
I think release notes section is important reminder to add that to PR desc.
I do not care much about the rest.
There was a problem hiding this comment.
Documentation + Release notes headers are useful and should be kept. Everything else often isn't very useful.
There was a problem hiding this comment.
Everything else often isn't very useful.
Good that we have agreement about this.
Re release notes --
i think that, at the moment of filing PR, it's hard to formulate good release notes. The PR content may change over time.
anyway, I think it's might be useful if it is filled in. -- Is it?
There was a problem hiding this comment.
Documentation + Release notes headers are useful and should be kept. Everything else often isn't very useful.
not sure what you'd want to remove?
There was a problem hiding this comment.
The boilerplate under "Description" and entirety of "Related issues, pull requests, and links".
There was a problem hiding this comment.
The boilerplate under "Description" and entirety of "Related issues, pull requests, and links".
I wish we could remove much more, but let's do baby steps.
I restored all other parts for now.
@hashhar PTAL
.github/pull_request_template.md
Outdated
| @@ -1,44 +1,5 @@ | |||
| <!-- Thank you for submitting a pull request! Find more information in our development guide at https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md and contact us on #dev in Slack. --> | |||
There was a problem hiding this comment.
Github has a builtin support for this:
There was a problem hiding this comment.
(screenshot from some other project)
There was a problem hiding this comment.
Does that actual work in the Trino project or does it need some specific setup @findepi ?
There was a problem hiding this comment.
It works automatically if you have a CONTRIBUTING.md for first time submitters.
.github/pull_request_template.md
Outdated
| ( ) Documentation PR is available with #prnumber. | ||
| ( ) Documentation issue #issuenumber is filed, and can be handled later. | ||
|
|
||
| ## Release notes |
There was a problem hiding this comment.
Documentation + Release notes headers are useful and should be kept. Everything else often isn't very useful.
It seems that pull request template is not serving the project well. It's apparently not filled in in majority of the PRs, also including PRs authored by maintainers. As a result, reviewers spend time eyeballing the template's text to check whether this is just a raw template or there is any information added. This commit removes a section of the pull request template that maintainers agree to remove. It is left for a future work to remove other parts of the pull request template.
findepi
force-pushed
the
findepi/simplify-pull-request-template-062825
branch
from
af3458b to
3000ae8
Compare
|
I think we should at least keep something to prompt people to add some details about what the PR is about, what kind of information we’re looking for, etc. Maybe it doesn’t need to be as structured as before, but it should be there in some form. |
agreed. That's why i left
|
There was a problem hiding this comment.
The fact that maintainers and users are not filling this all in can not be taken as evidence that the information is not useful. On the contrary .. when it is filled in the info is typically very useful to anyone reading the info including the maintainers who end up merging the PRs.
The three specific questions in the description section where suggested by @dain and are typically quick and easy to answer. I believe everyone should just try a bit harder to actually fill the information in.
The dedicated section for related issues and links is very useful for seeing these links in one place. Typically they show up later down when you link an issue or PR but you have to scroll down (sometimes very far) to find them. As such the central location is quite useful.
In general I want to reiterate that the template is there to help and it does NOT all have to be filled in at the very start. In fact it should evolve over time, and ideally be in good shape before merge so any maintainer who comes in to merge can quickly find all relevant information.
The comments in the template are there to help everyone fill the info out. Thats the only part I think we could potentially remove, but I also see no harm in it since it only shows up when editing the description of the PR.
Overall I strongly oppose the removal of all this information. We have had this template for a few months only and it has helped a lot with the release notes, documentation, and general verification and information about each PR.
|
I would argue that we should leave behind "If this change is user visible, how would you describe this change to a non-technical user?" I think that's an important bit of context which sometimes doesn't get conveyed in PR titles / descriptions, and it makes my job of editing/cleaning up release notes much easier if the note itself isn't great. If change is not user visible, then anyone can just skip. It's also been mentioned already, but we absolutely do need either that or a proposed release note, or both. I'd lean towards keeping those two things, but it would be especially harmful if we eliminated both, because we really shouldn't get rid of all context. I think I'm otherwise ok with removing most of it. The docs section tends to not get used (or used improperly), and if there's so much to fill out that it's an annoying obstacle/hurdle to submitting a PR, we can accept that we don't care as much about some of these things. By cutting it down and simplifying, I think it'll be easier to be a stickler and strongly encourage people to fill out the template, because 1-3 questions isn't nearly as bad. |
|
Another consideration: if the PR template isn't being used, we should ask ourselves if the issue is the template, or if the issue is that we haven't done a good enough job respecting it as the important context that it is. I do agree that it needs some changes, and I myself was going to propose some soon, but filling the template out is 2-3 minutes of work tops when you're submitting a PR (and 15-20 seconds for an internal/test change), and this seems like an overly-heavy hand blaming the template when we should probably start by doing a better job to encourage engineers and maintainers to use the template in the first place. |
There is no disagreement that useful information is useful, if it exists.
Typically organizational changes like this should have an improvement loop. How long more should we be trying before we can draw conclusions?
I disagree. Also, for a person who files many PRs, filling the template feels laborious. That's at least why i never bother filling it in. That's perhaps why many people do not bother filling it in.
@colebow that's a fair question.
Who's blaming anything? |
@colebow do want to give it a try? I can wait a bit before merging this. My requirement is: it should not take me more than 400 ms to understand that PR has no description. Wiping out the template is one way of doing it. |
|
Sorry, to be clear, when I say "blame," I'm not saying we're pointing at people and accusing them of doing a bad job. I think I agree that the PR template and how we use it needs some improvement, and the work needs to be done on the people side and/or on the template side. We need to determine which side of the equation is key in not providing us the full value we want it to, and if we're knocking the template out in this PR, that's implicitly asserting that the template was the issue (and was to "blame"). This could be perfectly valid, and I suspect to some extent that it is, but it might be a little hasty / not giving it a fair chance. Let me try to sit down and think about this, and if you're ok with it, I'll put up a proposal of my own later today. |
|
#13947 - here's my try. |
Let's assume people are at fault. Again, template is something we can change easily*. (well, we should be able to change template easily, if it didn't incur infinite amount of
We gave it 7 months, so i think it was fair chance.
thanks! let me take a look |
|
I think #13947 is a better alternative than this one, merged. as a follow-up, we can remove more :) |
It's apparently not filled in in majority of the PRs, also including
once authored by maintainers. As a result, reviewers spend time
eyeballing the template's text to check whether this is just a raw
template or there is any information added.