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

github: adding a pull request template #592

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@bobafetthotmail
Contributor

bobafetthotmail commented Dec 6, 2016

This text is used by GitHub to remind important things to people posting PRs through the GitHub's web interface. See here for more information https://github.com/blog/2111-issue-and-pull-request-templates

Loosely inspired by OpenWRT package feed's own pull request template
https://github.com/openwrt/packages/blob/master/.github/pull_request_template

Signed-off-by: Alberto Bursi alberto.bursi@outlook.it

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 6, 2016

feel free to add more requirements to the list in this template.

@pepe2k

This comment has been minimized.

Member

pepe2k commented Dec 6, 2016

This is really great!

I would also add/mention:

  • preferred approach is to use separate branch for every PR
  • short tutorial how to rebase and update PR (I see that many people have problems with that when they are asked for changes)

Cheers

@miltador

This comment has been minimized.

miltador commented Dec 6, 2016

@pepe2k, your points are good. But I think it would be also nice to have CONTRIBUTING.md file in the root of the project near readme, in which there will be described your points and maybe also other dedicated to contributing info (maybe from LEDE Wiki). GitHub has integration of this file and points user to read it before creating PR.

@diizzyy

This comment has been minimized.

Contributor

diizzyy commented Dec 6, 2016

I managed to help someone on IRC using this quick and ugly hack...
It's far from optimal but at least this person managed to do a successful pull request :-)
http://paste.ubuntu.com/23472999/

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 6, 2016

@miltador the openwrt packages feed has a CONTRIBUTING.md file we can also mostly copy. https://github.com/openwrt/packages/blob/master/CONTRIBUTING.md

Most of other stuff proposed is good for a wiki page, not a short reminder, thanks dizzyyy for that pastebin. Will make a wiki page soon-ish.

@wongsyrone

This comment has been minimized.

Contributor

wongsyrone commented Dec 7, 2016

I think <device-name> should be <arch-name>

@EricLuehrsen

This comment has been minimized.

Contributor

EricLuehrsen commented Dec 7, 2016

Maybe it could be something more direct and in the form of a check list.

Please check the following before making a pull request:
- Make each commit title "package*: what it fixes or adds"
-- Make commit tittle 50 char max, and line wrap commit body at 72.
-- Confirm your github account is attached to each commit (your personal github icon)
-- Use a unique branch for each pull request so like "package_feature."
- Make the pull request title "package*: summary of combined effect of commits"
-- Describe the problem solved or enhancement in the pull body
-- Describe the compile environment(s) you used in the pull body
-- Describe the test environment(s) you used in the pull body
- Seek additional guidance for developer submissions at: 
-- https://www.lede-project.org/docs/guide-developer/the-source-code#general_source_structure.
-- [github contributing.md]
-- [other]

*package also may be architecture, complete device, or tools component

Thanks for your contribution.
Please remove this message before posting the pull request.

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from 67389d1 to ab9144e Dec 8, 2016

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 8, 2016

updated to integrate some of the feedback.

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from ab9144e to 5885589 Dec 8, 2016

address

- all commits must be linked to your github account (you see your logo/avatar in front of them), if they aren't
linked you probably need to change the local git name or email in your PC to match the one on GitHub.

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

incorrect, SoB needs to match author field.

- commit descriptions must describe the problem solved or enhancement in the commit body

- all commits must end with "Signed-off-by: My Name <my@email.address>" where you write your real name and real email
address

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

s/must end with/must contain/

website)

- commit titles start with "<package name>:" for packages "<device architecture>: " for devices
- commit titles must state in short form what was fixed or added

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

s/titles/subject/

@@ -0,0 +1,23 @@
Thanks for your contribution to the LEDE project!
To help people review your contribution we ask you to follow some rules:

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

s/some/these/



- full patch submission rules are here
https://www.lede-project.org/docs/guide-developer/the-source-code#general_source_structure

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

s/rules/guidelines/

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from 5885589 to 7d0e899 Dec 8, 2016

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 8, 2016

updated.

the email in the Author field for PRs opened through GitHub is the GitHub's "primary email" in the settings, I found out in this PR when you told me to fix mine #471

address

- the name used in the "Signed-off-by:" line must be the same you write in "Name" field in GitHub settings, and/or
see in your PC with "git config user.name"

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

still incorrect. the author field and SoB need to match.

@blogic

This comment has been minimized.

Contributor

blogic commented Dec 8, 2016

the #471 comment is true if and only if changes are made via the UI. if you push a locally generated tree then the author field is not changed by github.

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from 7d0e899 to bfb6ebe Dec 8, 2016

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 8, 2016

updated.

I kept the instructions for fixing Author field as for most newbies it is auto-generated by git (either local or on GitHub) so they have no idea of what you mean by "Author field must match SoB" unless someone explains how to fix it.

@blogic

This comment has been minimized.

Contributor

blogic commented Dec 8, 2016

coming to think of this, the patch you propose does not add a template. it adds an instruction text. a template is, as the name suggests a template that needs to be filled out, like a blank form where you just fill in the fields.


-- if you are editing files and committing through GitHub, you must write your real name in the "Name" field in
GitHub settings and the email used in the "Signed-off-by:" must be your primary github email

This comment has been minimized.

@blogic

blogic Dec 8, 2016

Contributor

one of the main problems is that author and SoB do not match. adding an explanation that does not address one of the most common error does not make sense. i am not sure how many more time i need state that it is important to list this here. i have said it 4 times now. but i am happy to repeat myself even more often.

@blogic

This comment has been minimized.

Contributor

blogic commented Dec 8, 2016

adding a commit that explains how to add commits and then having that commit not follow that explanation seems odd. why is there no prefix on the subject line ?

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 8, 2016

coming to think of this, the patch you propose does not add a template. it adds an instruction text. a template is, as the name suggests a template that needs to be filled out, like a blank form where you just fill in the fields.

The same was done by Thess for the issue template, see here https://github.com/lede-project/source/blob/master/.github/issue_template
also for OpenWRT package repo it is both a message and a template (link in first message)

Afaik it's common on github to "misuse" the template feature this way as there is no other way to post a message to people making PRs.

adding a commit that explains how to add commits and then having that commit not follow that explanation seems odd. why is there no prefix on the subject line ?

because this is a file for github, not for a package or a device in LEDE, and the commit for the issue template did not have a prefix so I thought it was OK to not have one here too 4f40f22

one of the main problems is that author and SoB do not match. adding an explanation that does not address one of the most common error does not make sense.

What? Author field is auto-generated by git by combining name and email:
On Github's git it uses what you wrote in Name (or your github nickname if no Name is set, like the case with that last guy) and Github's "primary email".
On the committer's local repo it uses user.name and user.email git settings, you can override that with --author=" myname my.email@address", but most guides I've seen tell to set the user.name and user.email globally.

Newbie guides never mention "author field" and there is no such thing in git settings afaik so I have to tell how to set that properly in a document supposed to help newbies.

@blogic

This comment has been minimized.

Contributor

blogic commented Dec 8, 2016

NAK from me until the note is added stating that Author field and SoB line need to match as this is one of the most common errors next to

  • no prefix
  • missing description
  • missing SoB.
@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 8, 2016

Um, I already wrote in the template:

  • the Author field the must match the "Signed-off-by:" line:

Did you miss it or is it not clear?

@miltador

This comment has been minimized.

miltador commented Dec 8, 2016

coming to think of this, the patch you propose does not add a template. it adds an instruction text. a template is, as the name suggests a template that needs to be filled out, like a blank form where you just fill in the fields.

Agree. This actually can be done in a style of template that user should fill instead of removing the text. As a good example of such, look at this template: https://github.com/serverless/serverless/blob/master/.github/PULL_REQUEST_TEMPLATE.md
It is structured, has points to fill and also the list of checks that can include something like "Added Signed-off-by line" and so on.

@EricLuehrsen

This comment has been minimized.

Contributor

EricLuehrsen commented Dec 8, 2016

Prefix tools infrastructure with the TOOL.
github: add repo pull request template/message

Or prefix with the package/directory.
LEDE/source: add repo pull request template/message

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Dec 16, 2016

ok, done.

@bobafetthotmail bobafetthotmail changed the title from github: adding a github pull request template to github: adding a pull request template Dec 16, 2016

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Jan 11, 2017

If there are no more comments on this, I think it might make sense merging it.

@thagabe

This comment has been minimized.

Contributor

thagabe commented Jan 31, 2017

ping, will this be merged?

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Feb 5, 2017

@mkresin you seem to be referencing this commit quite a bit lately, can you merge this or are we waiting for @blogic ?

@stefanct

This comment has been minimized.

stefanct commented Feb 12, 2017

The build prefix is missing for general build changes that are not targeted at something in toolchain/ AFAICT.

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Feb 13, 2017

@stefanct I don't understand what you are saying. Can you explain it better? What you want me to do?

@hnyman

This comment has been minimized.

Contributor

hnyman commented Feb 13, 2017

The build prefix is missing for general build changes that are not targeted at something in toolchain/ AFAICT.

I don't understand what you are saying. Can you explain it better? What you want me to do?

He suggests that the prefix examples list should include also "build:" for commits like f938de7 and fe1e362 that target the build logic itself. Also "toolchain:" has been used in some similar cases, e.g. in b2c6672

@stefanct

This comment has been minimized.

stefanct commented Feb 13, 2017

@bobafetthotmail You are listing some possible prefixes here: c7cfdff#diff-f7162af36e86259e13bfbf7f0fc3eb48R11
I was suggesting that you add some more... what @hnyman said while I was typing ;)
However, toolchain: and build: seem to be distinct to me (there are build files outside of toolchain/.

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Feb 14, 2017

ok updated with the "build:" option for non-tool buildsystem changes.
I think a "toolchain" option is unnecessary as there is a "tool name" option already that will catch the tools in /toolchain and stuff in /include or in /scripts (if it is a tool) but this isn't my call anyway.

@pepe2k

This comment has been minimized.

Member

pepe2k commented Mar 3, 2017

Hello @bobafetthotmail,

I would like to move on with this one.
I spoke with @mkresin about the idea of having GitHub PR template inside the repository and few things came out:

  • we agreed that we need this guideline for submitters, but more like a general submission guideline, not only for GitHub PR
  • any kind of change in this template text would require a separate commit which we would like to avoid (next point explains why)
  • we don't keep documentation with sources anymore, so the best place for this would be Wiki (atm, docs/guide-developer/ looks like a good candidate for it, but feel free to suggest something else)
  • we both think that it should be somehow highlighted and easily available in Wiki menu (side navigation), not "hidden" like the "Submitting Patches" section now
  • when this will be available in Wiki, we can rework this template and instead of whole guideline text, include only short information and link to the Wiki so that in case of any changes in guideline, we won't need to make changes in code repository

Cheers,
Piotr

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Mar 3, 2017

Hm, ok for me. Will migrate bulk of the submission requirements to the wiki and make the required changes in a few days.

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from 68913cb to 2d25c3d Mar 5, 2017

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Mar 5, 2017

Updated, the wiki article is here https://lede-project.org/docs/guide-developer/contribution (it is accessible from "Contributor's Guide" link on the sidebar on the left)

@mkresin

This comment has been minimized.

Contributor

mkresin commented Mar 5, 2017

Excellent work so far.

Any opinions on renaming:

  • "contributers guide" to "Submitting patches"
  • "Bug reports" to "Reporting bugs"

IMHO it more descriptive and might help people to find what they are looking for.

@EricLuehrsen

This comment has been minimized.

Contributor

EricLuehrsen commented Mar 5, 2017

I would like to see a short form underneath like OpenWrt packages, but this has been run to long. Lets just merge this simple piece of literature as is now and be done with it.

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Mar 5, 2017

changed the titles on wiki as suggested. (the page link does not change)

@pepe2k

This comment has been minimized.

Member

pepe2k commented Mar 6, 2017

@bobafetthotmail thank you for work on this!

One thing:

  • redable -> readable (typo?)

Also, it's probably not related to the PR that much, but I will just drop it here.

I have some problem with the Wiki left navigation menu: when I'm in Submitting patches section, the Documentation menu is expanded. Is it some kind of bug/mistake in the menu/pages hierarchy? This is how it looks now:

2017-03-06_090115.jpg

Cheers,
Piotr

github: adding a pull request template
This text is used by GitHub to remind important things to
people sending PRs through the GitHub's web interface.
See here for more information
https://github.com/blog/2111-issue-and-pull-request-templates

It links to the wiki page about submission rules.

Signed-off-by: Alberto Bursi <alberto.bursi@outlook.it>

@bobafetthotmail bobafetthotmail force-pushed the bobafetthotmail:patch-7 branch from 2d25c3d to eb05488 Mar 6, 2017

@bobafetthotmail

This comment has been minimized.

Contributor

bobafetthotmail commented Mar 6, 2017

redable -> readable (typo?)

Sigh, respinned the patch again, thanks.

I have some problem with the Wiki left navigation menu:

That's because the page is (was) technically in Developer Guide "namespace thing" in the wiki, see the (old) URL?
https://lede-project.org/docs/guide-developer/contribution
So the wiki expands that menu like when you are in any other article placed under quickstart, user guide or developer guide.

Since I had to fix that typo anyway, I also moved the page to root wiki "namespace" (and changed the link accordingly) so it won't open the menu like that.

@pepe2k

This comment has been minimized.

Member

pepe2k commented Mar 6, 2017

@mkresin, @blogic, what do you think about thist now?

@mkresin

This comment has been minimized.

Contributor

mkresin commented Mar 6, 2017

@pepe2k fine with me. Feel free to merge the PR and hopefully it will save us some work.

@pepe2k pepe2k self-assigned this Mar 8, 2017

@pepe2k

This comment has been minimized.

Member

pepe2k commented Mar 8, 2017

@bobafetthotmail and all others involved in work on this, thank you! I have merged this into my staging tree, with tiny change in commit subject.

@pepe2k pepe2k closed this Mar 8, 2017

@bobafetthotmail bobafetthotmail deleted the bobafetthotmail:patch-7 branch May 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment