Add PR template #146

kottmann · 2017-03-03T10:49:14Z

Addresses issue #139.

analytically · 2017-03-03T14:57:20Z

.github/PULL_REQUEST_TEMPLATE.md

+
+- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?
+
+- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?


should we enable branch protection instead of requesting this?

You can protect the master branch against force push, but this simply tells the contributor to rebase against the latest master branch. This is there to ensure that the PR can be merged without conflicts.

The master branch in JanusGraph is already protected against forced pushes, and @kottmann makes a good point that this is orthogonal — we need to make sure it will be a clean merge.

analytically · 2017-03-03T14:57:42Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Is your initial contribution a single, squashed commit?
+
+### For code changes:
+- [ ] Have you ensured that the full suite of tests is executed via mvn clean install at the root janusgraph folder?


-1 CI will catch this, it's automated

In general I believe it is good to run the test locally before sending a PR. I will remove this line.

Travis does not currently run core tests in all modules reliably and doesn't run TinkerPop tests at all. Where relevant I think PRs should include a description of the modules tested and which suites were run. Otherwise relevant modules should be tested as part of the PR review.

jerryjch

We recently delivered via PR #106 a development process doc. I wonder how what is here relates to that doc.

A side question. Where the development process doc would show up to users? Is there a link to it?
https://github.com/JanusGraph/janusgraph/blob/master/docs/development.txt

kottmann · 2017-03-06T09:34:05Z

@jerryjch You need to explain to contributors how things are done here. Most projects have this document on their website and not in their reference documentation. Currently - for someone new- that is the CONTRIBUTING.md file, I would strongly suggest to consolidate this or reference from there to the development.txt file.

Thew new .github/CONTRIBUTING.md (which the user is pointed to during the creation of his PR) currently points to the old CONTRIBUTING.md which I renamed to getting involved.

I think the PR template aligns nicely with your development.txt. So it repeats things and has some additional items (e.g. about LICENSE).

jerryjch · 2017-03-07T06:03:55Z

Ok @kottmann , sounds reasonable.

kottmann · 2017-03-09T14:01:14Z

Anything I should change here?

pluradj · 2017-03-14T04:13:06Z

+1 LGTM

@jerryjch do you have a vote on this?

mbrukman · 2017-03-14T04:45:11Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Is there an issue associated with this PR? Is it referenced 
+     in the commit message?
+
+- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?


Instead of putting it in the title, please put the issue # in the body. I've edited a few issues that do the former to do the latter, and here's why:

if the issue # is in the title, it looks confusing, because GitHub also appends the issue # to the title, so it looks like this PR addresses issue #A #B where #A is the issue you're addressing and #B is the auto-assigned number for this PR.

the #A in the title is not auto-linked, so you can't click on it to visit the issue

the issue #A does not automatically get a reference to the fact that PR #B is going to address it

In contrast, if you put addresses issue #A into the PR body, all of the above caveats are reversed.

Hmm, I looked at one of my private repositories and didn't see what you describe here.

If the issue is in the title/subject line it is shown in the commit history only once. If it is in the body the title doesn't contain the issue number. We have to be careful, this speaks about the git commit message not the title of the PR on Github.

The issue number in the title is also click-able and linked.

I'm talking about this:

Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?

I am saying that putting the issue # at the end of a PR title is confusing, because of the reasons above: it's not auto-linked, and GitHub appends the PR # to the end, so there are two numbers in the title: the issue it's addressing, and the PR itself.

I am suggesting that instead of putting the issue # in the PR title, we put it in the PR description instead, which addresses all of the above issues. Having it in the commit also makes sense, but that's not what I'm referring to.

Ok, I misunderstood you here, after my last change it now says "PR body" instead of "PR title".

mbrukman · 2017-03-14T04:45:41Z

.github/PULL_REQUEST_TEMPLATE.md

+
+- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?
+
+- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?


Please add code formatting to master: master

mbrukman · 2017-03-14T04:45:58Z

.github/PULL_REQUEST_TEMPLATE.md

+### For all changes:
+- [ ] Is there an issue associated with this PR? Is it referenced 
+     in the commit message?
+


Drop blank line?

A line should end on a line feed. Are you sure you want it dropped? A few tools work a tiny bit nicer when there is a new line at the end of the file, e.g. no git "No newline at end of file" message in diffs, adding a line won't change the last line, etc.

This is not the end of a file, there are other items in this list below. Also, I noticed in a different list, there were no blank lines between items, so I figured we may want to be consistent.

Right, then let me fix this.

mbrukman · 2017-03-14T04:46:02Z

.github/PULL_REQUEST_TEMPLATE.md

+     in the commit message?
+
+- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?
+


Drop blank line?

mbrukman · 2017-03-14T04:46:31Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Is your initial contribution a single, squashed commit?
+
+### For code changes:
+- [ ] Have you written or updated unit tests to verify your changes?


or -> and/or

Could be both add new ones and update existing ones.

mbrukman · 2017-03-14T04:47:09Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
+
+### Note:
+Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.


s/travis-ci/Travis CI/

mbrukman · 2017-03-14T04:49:25Z

README.md

@@ -31,5 +31,5 @@ The [project homepage](http://janusgraph.org) contains more information on Janus

 ## Contributing

-Please see [`CONTRIBUTING.md`](CONTRIBUTING.md) for more information, including


Why are we renaming CONTRIBUTING.md to GETTING_INVOLVED.md?

If there's a file called CONTRIBUTING or CONTRIBUTING.md at the root of the repo, GitHub handles it specially by auto-linking it.

The CONTRIBUTING.md file is the one shown to the user when submitting a PR, I renamed the existing one to GETTING_INVOLVED.md and added a new CONTRIBUTING.md (part of this PR). Maybe have a look at both versions and see if you think it makes sense as well.

@kottmann @mbrukman In this PR why is ./.github/CONTRIBUTING.md rooted at ./.github and not ./?

@kottmann any updates on this? seems that this has gotten stalled. as @amcp mentions above, it seems like the files are in the wrong spot.

https://github.com/blog/2111-issue-and-pull-request-templates specifically notes that you can keep the PR and issue template files in .github subdir to keep the root of the repo leaner by having fewer files there.

I would like to propose:

CONTRIBUTING.md stays at the root of the repo because it's generally useful to everyone

PR and issue template files go into .github because they're GitHub-specific workflow items

@amp, @pluradj — thoughts?

jerryjch · 2017-03-14T16:42:06Z

In the https://github.com/JanusGraph/janusgraph/blob/master/docs/development.txt, we have:
"Be sure to include the relevant issue number in the title of your pull request ..."
I think @twilmes has a good example of it in this PR:
#159
I like the format. Should we all follow it?

kottmann · 2017-03-14T20:17:48Z

@jerryjch PR 159 is a bad example, he didn't squash the commits, the commit message is not containing the issue number, and his subject line (first line of commit message) is too long.

@mbrukman I couldn't see the behaviour on Github regarding the issue number in the title like you described it. Issue number was only shown once and behaved like expected.

Anyway, just decide how you prefer it, in case you want to issue number in the body I will change the development.txt with this PR as well to get it unified.

jerryjch · 2017-03-14T20:59:17Z

Ok, I should have been more clear. I meant the "Issue 155: ..." format on PR #159
It looks more consistent with the "JIRA number: ..." format many Apache projects have.
But anyway, I am fine with any meaningful approach.

kottmann · 2017-03-15T10:42:30Z

@mbrukman now all your feedback should be addressed.

sjudeng · 2017-03-31T00:45:24Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Is there an issue associated with this PR? Is it referenced in the commit message?
+- [ ] Does your PR body contains #xyz where xyz is the issue number you are trying to resolve?
+- [ ] Has your PR been rebased against the latest commit within the target branch (typically ```master```)?
+- [ ] Is your initial contribution a single, squashed commit?


Is this saying it's necessary to have a single squashed commit for all PRs? I think it is sometimes helpful to separate out commits. Examples include multiple authors collaborating on a PR, separating documentation and formatting updates from feature updates and more generally for larger features where the work has been broken up into meaningful commits.

Considering the difficulty we had merging a large CR in the past (we made sure to give @dylanht credit), it does make sense to allow multiple commits sometimes. Seems that it would make sense for hosted branches (major feature branches) more than PR by individuals.

amcp

took a look

amcp · 2017-04-05T05:38:52Z

.github/PULL_REQUEST_TEMPLATE.md

+- [ ] Is there an issue associated with this PR? Is it referenced in the commit message?
+- [ ] Does your PR body contains #xyz where xyz is the issue number you are trying to resolve?
+- [ ] Has your PR been rebased against the latest commit within the target branch (typically ```master```)?
+- [ ] Is your initial contribution a single, squashed commit?


Considering the difficulty we had merging a large CR in the past (we made sure to give @dylanht credit), it does make sense to allow multiple commits sometimes. Seems that it would make sense for hosted branches (major feature branches) more than PR by individuals.

amcp · 2017-04-05T05:40:36Z

README.md

@@ -31,5 +31,5 @@ The [project homepage](http://janusgraph.org) contains more information on Janus

 ## Contributing

-Please see [`CONTRIBUTING.md`](CONTRIBUTING.md) for more information, including


@kottmann @mbrukman In this PR why is ./.github/CONTRIBUTING.md rooted at ./.github and not ./?

sjudeng · 2017-07-30T23:20:41Z

@kottmann It looks like there are just a couple remaining issues on this. If it's alright with you I can add another commit to your branch to address the remaining requests.

amcp · 2017-07-31T01:05:10Z

@sjudeng this also needs to be rebased

sjudeng · 2017-07-31T01:20:19Z

@mbrukman, @amcp can you review this again? GitHub does allow the files to be rooted in .github to avoid clutter (see this post) but I moved everything back to the project root for now and we can revisit whether to move things under .github separately.

@amcp I merged to avoid needing to force push and overwrite the original commit. @kottmann feel free to rebase and squash.

kottmann · 2017-07-31T08:36:48Z

@sjudeng let me squash and rebase what is here now

sjudeng · 2017-08-13T13:32:01Z

@mbrukman, @amcp It appears that all requested changes have been addressed here. Can you confirm?

sjudeng · 2017-09-05T20:13:26Z

@mbrukman, @amcp - Are you okay with this getting merged? I think all your requests have been addressed.

mbrukman · 2017-09-05T20:15:43Z

PULL_REQUEST_TEMPLATE.md

+
+### For all changes:
+- [ ] Is there an issue associated with this PR? Is it referenced in the commit message?
+- [ ] Does your PR body contains #xyz where xyz is the issue number you are trying to resolve?


s/contains/contain/

"Does ... contain"

mbrukman · 2017-09-05T20:16:08Z

PULL_REQUEST_TEMPLATE.md

+### For all changes:
+- [ ] Is there an issue associated with this PR? Is it referenced in the commit message?
+- [ ] Does your PR body contains #xyz where xyz is the issue number you are trying to resolve?
+- [ ] Has your PR been rebased against the latest commit within the target branch (typically ```master```)?


For single-line code font, please use a single backtick, not triple (that's only for multiline).

mbrukman · 2017-09-05T20:17:31Z

PULL_REQUEST_TEMPLATE.md

+### For code changes:
+- [ ] Have you written and/or updated unit tests to verify your changes?
+- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? 
+- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file in janusgraph folder?


If we're being specific, should this be s/LICENSE/LICENSE.txt/ ?

Similarly for s/NOTICE/NOTICE.txt/ on the next line.

mbrukman · 2017-09-05T20:18:21Z

PULL_REQUEST_TEMPLATE.md

+### For code changes:
+- [ ] Have you written and/or updated unit tests to verify your changes?
+- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? 
+- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file in janusgraph folder?


By "in janusgraph folder", does this mean "root of this repository"? I understand git clone would create the janusgraph folder, but that's a user specified name, so it may be ambiguous, but root of the repo probably isn't.

Similarly on the next line.

mbrukman · 2017-09-05T20:20:50Z

PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,23 @@
+Thank you for contributing to JanusGraph.


To avoid adding yet more files to the root of the repo, what about moving this file to a .github subdirectory, since this is a GitHub-specific workflow? We might also add an ISSUE_TEMPLATE.md in the future, so this will help keep the root of the repo leaner.

https://github.com/blog/2111-issue-and-pull-request-templates says both top-level files and files in .github are supported.

I would recommend keeping CONTRIBUTING.md at the root of the repo since it's independent of GitHub and applicable to everyone.

Thoughts?

@mbrukman It was originally in .github but was moved back as part of review (see this comment and this comment). I'm okay with changing it back if that makes more sense.

Sorry, missed the original thread at the time it was pointed out, and looks like it wasn't quite resolved with a consensus. Added a comment on that thread as well with my proposal.

Should I move the PULL_REQUEST_TEMPLATE.md back into the .github folder?

@mbrukman Is it okay to just move it back to .github? I'm definitely okay with it (I just want this PR finally merged). @kottmann Sorry I moved it in the first place.

@kottmann, @sjudeng — yes, please.

janusgraph-bot · 2017-09-05T21:48:04Z

Committer of one or more commits is not listed as a CLA signer, either individual or as a member of an organization.

mbrukman · 2017-09-05T22:03:28Z

@kottmann — the CLA bot is really pedantic, and you're using a combination of "Joern Kottmann" and "Jörn Kottmann" as your name as "author" and "committer", but only one of them is listed in the CLA signers file.

I'm happy to add the second spelling of your name to the CLA file to fix this problem going forward, if you'd like. Alternatively, you'll have to be really careful about using a single spelling of your name everywhere, which is probably more trouble than it's worth, given that both of those are equally valid.

kottmann · 2017-09-05T22:12:28Z

@mbrukman sorry for that, please add the second spelling to the file, to speed things up here I changed author and comitter to "Joern Kottmann".

mbrukman

Minor comment about the new path (as a result of moving the file). Thanks!

mbrukman · 2017-09-05T23:06:57Z

CONTRIBUTING.md

@@ -159,6 +163,8 @@ Go to the [JanusGraph repository](https://github.com/JanusGraph/janusgraph) and
 you should see that it will offer you a chance to compare your recently-pushed
 branch to the current `master` of JanusGraph and subit a PR at the same time.

+Review the [PR check list](PULL_REQUEST_TEMPLATE.md) for criteria for acceptable contributions.


This should now point to the new location of the file under .github.

sjudeng · 2017-09-05T23:18:04Z

@kottmann If you push another update can you add [skip ci] to your commit message to avoid triggering a Travis build?

[skip ci]

All requests have been addressed

Add PR template

janusgraph-bot added the cla: yes This PR is compliant with the CLA label Mar 3, 2017

analytically reviewed Mar 3, 2017

View reviewed changes

kottmann force-pushed the 139 branch from 7e992e3 to b0a9125 Compare March 3, 2017 15:08

jerryjch self-requested a review March 4, 2017 19:36

jerryjch reviewed Mar 4, 2017

View reviewed changes

mbrukman changed the title ~~Add PR template #139~~ Add PR template Mar 7, 2017

pluradj self-requested a review March 14, 2017 04:31

pluradj approved these changes Mar 14, 2017

View reviewed changes

mbrukman requested changes Mar 14, 2017

View reviewed changes

kottmann force-pushed the 139 branch from b0a9125 to c60ee8c Compare March 14, 2017 09:24

kottmann force-pushed the 139 branch from c60ee8c to ced9b59 Compare March 15, 2017 10:38

sjudeng reviewed Mar 31, 2017

View reviewed changes

amcp previously requested changes Apr 5, 2017

View reviewed changes

sjudeng mentioned this pull request Apr 10, 2017

Add ability to await on multiple statuses #192

Merged

sjudeng force-pushed the 139 branch from e5f89a4 to 03d9857 Compare July 31, 2017 01:11

kottmann force-pushed the 139 branch from 03d9857 to ba60133 Compare July 31, 2017 08:36

mbrukman requested changes Sep 5, 2017

View reviewed changes

kottmann force-pushed the 139 branch from ba60133 to d581b84 Compare September 5, 2017 21:47

janusgraph-bot added cla: no This PR is not compliant with the CLA and removed cla: yes This PR is compliant with the CLA labels Sep 5, 2017

kottmann force-pushed the 139 branch from d581b84 to 95e02c1 Compare September 5, 2017 22:11

janusgraph-bot added cla: yes This PR is compliant with the CLA and removed cla: no This PR is not compliant with the CLA labels Sep 5, 2017

mbrukman mentioned this pull request Sep 5, 2017

Add alternative spelling of @kottmann's name JanusGraph/legal#58

Open

mbrukman approved these changes Sep 5, 2017

View reviewed changes

Add PR template JanusGraph#139

f11845a

[skip ci]

kottmann force-pushed the 139 branch from 95e02c1 to f11845a Compare September 6, 2017 08:23

sjudeng merged commit 7d5779a into JanusGraph:master Sep 6, 2017

bwatson-rti-org pushed a commit to bwatson-rti-org/janusgraph that referenced this pull request Mar 9, 2019

Merge pull request JanusGraph#146 from kottmann/139

e9cda90

Add PR template

micpod pushed a commit to micpod/janusgraph that referenced this pull request Nov 5, 2019

Merge pull request JanusGraph#146 from kottmann/139

42477cf

Add PR template


		- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?

		- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?

		in the commit message?

		- [ ] Does your PR title ends with #xyz where xyz is the issue number you are trying to resolve?

		@@ -31,5 +31,5 @@ The [project homepage](http://janusgraph.org) contains more information on Janus

		## Contributing

		Please see [`CONTRIBUTING.md`](CONTRIBUTING.md) for more information, including

Add PR template #146

Add PR template #146

Conversation

kottmann commented Mar 3, 2017 • edited by mbrukman Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryjch left a comment

Choose a reason for hiding this comment

kottmann commented Mar 6, 2017 • edited Loading

jerryjch commented Mar 7, 2017

kottmann commented Mar 9, 2017

pluradj commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryjch commented Mar 14, 2017

kottmann commented Mar 14, 2017

jerryjch commented Mar 14, 2017

kottmann commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng commented Jul 30, 2017

amcp commented Jul 31, 2017

sjudeng commented Jul 31, 2017

kottmann commented Jul 31, 2017

sjudeng commented Aug 13, 2017

sjudeng commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janusgraph-bot commented Sep 5, 2017

mbrukman commented Sep 5, 2017

kottmann commented Sep 5, 2017

mbrukman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng commented Sep 5, 2017

kottmann commented Mar 3, 2017 •

edited by mbrukman

Loading

kottmann commented Mar 6, 2017 •

edited

Loading