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

Remove template for GitHub pull requests #1154

Merged
merged 1 commit into from
Feb 21, 2017

Conversation

fingolfin
Copy link
Member

Please make sure that this pull request:

  • is submitted to the correct branch (the stable branch is only for bugfixes)
  • contains an accurate description of changes for the release notes below
  • provides new tests or relies on existing ones
  • correctly refers to other issues and related pull requests

Tick all what applies to this pull request

  • Adds new features
  • Improves and extends functionality
  • Fixes bugs that could lead to crashes
  • Fixes bugs that could lead to incorrect results
  • Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

This PR removes our template for GitHub PRs. I find that template useless, and always remove it when submitting a PR. This one is an exception, to demonstrate what the template looks like, and also to remind of the fact that information in it need not be accurate anyway: You must check whether a branch was submitted to the right branch anyway -- if we want to keep track of that, let's use a label. Most other checkboxes can also be handeled by labels.

And finally, if information is missing in a PR, we just ask for it (if we keep asking the same questions, we can start using "Saved Replies", see https://help.github.com/articles/working-with-saved-replies/

@codecov
Copy link

codecov bot commented Feb 18, 2017

Codecov Report

Merging #1154 into master will decrease coverage by -0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1154      +/-   ##
==========================================
- Coverage   59.24%   59.23%   -0.01%     
==========================================
  Files         434      434              
  Lines      231096   231096              
==========================================
- Hits       136904   136899       -5     
- Misses      94192    94197       +5
Impacted Files Coverage Δ
lib/tietze.gi 41.85% <ø> (-0.74%)
lib/stbcrand.gi 89.97% <ø> (-0.5%)
lib/straight.gi 25.28% <ø> (-0.42%)
lib/grppcint.gi 99.32% <ø> (+0.67%)
lib/polyrat.gi 59.46% <ø> (+0.75%)
lib/csetperm.gi 91.35% <ø> (+3.7%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 124544f...b419c9f. Read the comment docs.

@olexandr-konovalov
Copy link
Member

I am perfectly happy with people just removing the template text and starting to write their own description. But I do think that the template is useful, especially for new contributors, and for changes that are worth describing in release notes, so I don't want to see it gone completely. I've used it myself in #1144 and find it helpful.

However, I agree that it's annoying that it is too long, and it will be useful to squeeze it. For example:

**This is just a reminder, feel free to delete it**: Please make sure to submit pull request 
to the correct branch (the stable branch is only for bugfixes) and provide tests.

**Only for changes to be listed in release notes**: give an accurate description and tick all what applies:
 -- [ ] Adds new features
 -- [ ] Improves and extends functionality
 -- [ ] Fixes bugs that could lead to crashes
 -- [ ] Fixes bugs that could lead to incorrect results
 -- [ ] Fixes bugs that could lead to break loops

@fingolfin
Copy link
Member Author

fingolfin commented Feb 18, 2017

I don't think that second section is useful at all. First off, it requires the submitter to know (or decide?) whether the PR should be listed in the releases notes. This is already where it goes wrong, IMHO.
Next, the information therein could be collected via labels, and IMHO better, because labels are searchable. E.g. there could be an "add-to-release-notes" label; then it'd become trivial to list all merged PRs that have this label. As for the other checks, we already have labels for "new feature", "enhancement", "bug", "crashes", "wrong result", "unexpected error".

Of course these are not applied consistently -- just as those checkbox are not consistently checked.

@olexandr-konovalov
Copy link
Member

OK, the 2nd section addresses the following problem: when there is a time to collate all descriptions for the release announcement, many of them are either missing or written informally, and it requires more efforts to put them together than it should be. An alternative is to drop that completely, and refer to the list of issues/PRs under release milestone, but that would contain a lot of noise (see e.g. here for coming GAP 4.9).

I agree with the argument that it may be not known to the submitted in advance. So asking for a description and checkboxes could be moved to one of the "saved replies" to be asked later. The label "for-release-notes" sounds like a good alternative idea.

So, shall we then reduce the template to have a last-minute reminder about correct branch and also about tests?

**This is just a reminder, feel free to delete it**: Please make sure to submit pull request 
to the correct branch (the stable branch is only for bugfixes) and provide tests.

If there is a strong support for deleting this template completely, please go ahead - I will not merge this myself though ;-)

@hungaborhorvath
Copy link
Contributor

I used the template exactly once, and I recall that it took too much time (relatively) to fill it out. In all other cases I just got annoyed and removed all text.

@olexandr-konovalov
Copy link
Member

So, shall we leave just this text then?

**This is just a reminder, feel free to delete it**: Please make sure to submit pull request 
to the correct branch (the stable branch is only for bugfixes) and provide tests.

@markuspf
Copy link
Member

To be completely honest, I am still in favour of just removing the template entirely.

@olexandr-konovalov
Copy link
Member

Ok, fine. We can always restore it if need be. But we do must keep some checklist and maintain discipline while merging PRs: branch, tests, description, closing related issues, etc.

@markuspf markuspf merged commit ebf865d into gap-system:master Feb 21, 2017
@fingolfin fingolfin deleted the mh/remove-pr-template branch February 27, 2017 14:28
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

Successfully merging this pull request may close these issues.

4 participants