PULL_REQUEST_TEMPLATE.md: Request PRs describe themselves

[ehmry]

There are a lot of PRs for updates that don't make it easy to find
out what changes might be breaking and lots of PRs for new packages
that don't describe what the new packages is or does.

[IvarWithoutBones]

Isn't that what meta is for? A brief description and link to the homepage should already be present for all new packages.

The changelog is also already a meta attribute, unless we explicitly know a change might be dangerous I would say that's good enough. Most upstream package changes are not very relevant to us.

[ehmry]

Isn't that what meta is for?

If you don't take the time to copy-paste a few lines from meta into the PR then you are wasting other peoples' time by making them click though github dialogs.

If someone can't be bothered to demonstrate that they understand the PR that they are making, we cannot hold them accountable to what they are requesting, nor can we hold whoever merged the PR responsible for more than looking at a few checkboxes.

[mweinelt]

That's scratching a real itch of mine, thank you.

[mweinelt]

The commit should probably lead with PULL_REQUEST_TEMPLATE.md:.

[SuperSandro2000]

If you don't take the time to copy-paste a few lines from meta into the PR then you are wasting other peoples' time by making them click though github dialogs.

If I want to review the PR I need to go there anyway so this is not an inconvenience for me. Personally I wouldn't mind if meta.changelog is always used as long as you can just click on the link without substituting version.

I also barely use the PR template and also don't care to much if people don't fill it out.

I guess the comment is useful but I don't really have a strong opinion about it.

[mweinelt]

If I want to review the PR I need to go there anyway so this is not an inconvenience for me. Personally I wouldn't mind if meta.changelog is always used as long as you can just click on the link without substituting version.

We are 170 committers and a much larger number of contributors. Linking to relevant information saves me alot of time, so I appreciate this formalization of expectations.

I also barely use the PR template and also don't care to much if people don't fill it out.

Disappointing.

[mweinelt]

Isn't that what meta is for? A brief description and link to the homepage should already be present for all new packages.

Totally valid, but I'd like for this metadata to appear directly in the PR, scrolling through unrelated source code is annoying. If we can automate that meta.changelog gets evaluated (it often uses substitutions) and posted that'd be fine, too. I'm not aware meta.changelog gets used at all these days, expect maybe by r-ryantm.

[SuperSandro2000]

We are 170 committers and a much larger number of contributors. Linking to relevant information saves me alot of time, so I appreciate this formalization of expectations.

If it helps you and possible others than it might be a good call to add it. I just wanted to add my 2c and be honest that I wouldn't get much use out of it.

If we can automate that meta.changelog gets evaluated (it often uses substitutions) and posted that'd be fine, too. I'm not aware meta.changelog gets used at all these days, expect maybe by r-ryantm.

That would be nice, too and it evens eliminates my point with variable substition. Maybe you could even edit the PR description to not generate comment notifications.

[ckiee]

<mweinelt> We are 170 committers and a much larger number of contributors. Linking to relevant information saves me alot of time, so I appreciate this formalization of expectations.

I've reviewed a few PRs recently and I double check the info is right in the derivation's nix file anyway so it being automated would be nice.

<SuperSandro2000> That would be nice, too and it evens eliminates my point with variable substition. Maybe you could even edit the PR description to not generate comment notifications.

We could copy matrix-react-sdk a bit, example: matrix-org/matrix-react-sdk#7086

<mweinelt> I'm not aware meta.changelog gets used at all these days, expect maybe by r-ryantm.

I think I saw it recently.

[IvarWithoutBones]

Totally valid, but I'd like for this metadata to appear directly in the PR, scrolling through unrelated source code is annoying. If we can automate that meta.changelog gets evaluated (it often uses substitutions) and posted that'd be fine, too. I'm not aware meta.changelog gets used at all these days, expect maybe by r-ryantm.

That's fair, I personally think it's not a big deal to copy paste the link from meta since I'd already be looking at the diff when reviewing PRs, but I'm all for making that information more accesible.

As for the changelog though, I don't think it makes a lot of sense to require contributers to write one out if nothing has changed in the build process. If a package update just includes internal non-breaking changes I fail to see how a list of those is relevant to nixpkgs maintainers. Many projects don't have a changelog upstream, having to go through the projects history to write one yourself sounds like a burden to maintainers with no real benefit. I do think this would be nice if things have changed from a packaging perspective though, or a change is known to be potentially dangerous.

Also, how would this work if a single PR includes multiple new packages? Dependencies are often included in the same pull, should the author add links/descriptions for everything?

That would be nice, too and it evens eliminates my point with variable substition. Maybe you could even edit the PR description to not generate comment notifications.

Sounds like a very interesting idea 👀

[mweinelt]

I'm not at all asking people to come up with their own changelog to substitute a missing upstream one. Most relevant projects that have reverse dependencies do have changelogs though and thereby a helpful indication of breaking changes to watch out for.

[piegamesde]

I think I saw it recently.

@ckiee you've seen the author of that PR? ^^

[IvarWithoutBones]

I'm not at all asking people to come up with their own changelog to substitute a missing upstream one.

Ah, I misinterpeted this a bit than. Maybe we should make that more clear in the template, although it might just be me 😅

[ckiee]

I think I saw it recently.

@ckiee you've seen the author of that PR? ^^

Yeah, it makes it a bit more ironic (:

Came straight out of my browser history though.

[mweinelt]

My point was not about people setting meta.changelog, but instead about it being evaluated and annotated in some useful fashion. But that is clearly out of scope for this PR.

The wording still needs to be fixed here #162790 (review).

[roberth]

Don't the commit messages already describe the change?
At least a motivation for the changes is new information.

I know that a motivation is a bit redundant for stuff like package updates, but for, say, changes to lib or nixos, the motivation is important for understanding the context and why the author wants to make the changes.

[ckiee]

Have a look at the thread in my review. Really both options are so similar that it might as well be decided by a PRNG.

[piegamesde]

Don't the commit messages already describe the change?

Yes, but many PRs have multiple commits and therefore a description that summarizes them all is nice to have. Also, most people automatically state why they are doing things when asked about describing, provided that they have a motivation worth stating.

[ehmry]

I've updated the text.

I don't think anyone is suggesting that we enforce the template, so the people who thought that "Motivation" was optional can just ignore the new text, and we can ignore their complaints.