Handling pull requests

[Jim Blake]

I have questions, based on the developer’s call on Tuesday.

 

1. What should be in a pull request? In other words, how does requester know what is expected? How do the reviewers know how to judge the request?
   1. We said in the meeting that a  JIRA issue is required.
   2. Do we require unit tests? Is a verbal test procedure acceptable, in lieu of coded tests?
   3. Do we require comments in the code? Do we require any other documentation?
   4. Are there conventions for coding style, such as naming conventions, formatting conventions, use of private vs. public members, etc.?
   5. What if the pull request modifies code which did not meet the standards criteria? Is the requester obliged to bring that code up to standard, as a way of reducing overall technical debt?
2. What happens when a pull request is created?
   1. Who is expected to respond to the requester? What happens if all of the committers ignore the request?
   2. We talked about voting on a pull request, and there are some loose guidelines here: https://wiki.duraspace.org/display/VIVO/Community+Decision+Making, but who calls for a vote? How much time is allowed for a vote? What happens if nobody votes?
3. What does a review consist of?
   1. Is it OK for a reviewer to just look at the file names and say “looks fine”?
   2. Should the reviewer examine each line of code?
   3. If there is a verbal test procedure, should the reviewer perform the tests?
   4. Does it make sense for someone to vote on a pull request if they have not reviewed it?
4. (et cetera, ad infinitum)…

 

It seems to me that we need:

1. A wiki page that begins “A pull request should include…”.
2. A wiki page that begins “When a pull request is received…”.
3. References to these pages from README.md at https://github.com/vivo-project/VIVO (thanks, Christian)

 

Is this material already available, and I am not aware of it?

 

Andrew, does Fedora have pages analogous to these?

 

Drowning in questions,

Jim

[Andrew Woods]

Hello Jim,

I agree that these are important questions to be answered as we move towards a community-driven process for managing maintenance and development of the codebase. Along those lines, it is important that the VIVO committers ultimately shape and agree on such policies.

I can suggest some possibilities:

1. GitHub has a built-in capability to present a template when a contributor opens a pull-request. Here is an example of such a template used in the Fedora project:

https://github.com/fcrepo4/fcrepo4/blob/master/.github/PULL_REQUEST_TEMPLATE.md

1b. There is a serious unit/integration testing conversation to be had. My general opinion is that unit tests are great for test-driven-development, but integration tests are what ensure the application works as expected over time.

1c. Documentation... yes, both in the code and in the wiki (as appropriate). I believe it is the documentation that opens the project to the broader community of contributors.

1d. Both DSpace and Fedora have documented coding style-guides that could be used as reference:

https://wiki.duraspace.org/pages/viewpage.action?pageId=90967266

https://wiki.duraspace.org/display/FF/Code+Style+Guide

It is also worth noting that the Maven Checkstyle plugin does a good job of enforcing whatever styles we require.

https://maven.apache.org/plugins/maven-checkstyle-plugin/

2. Responding to pull-requests: This is again a question on which the project committers need to decide... since the committers are the ones who ultimately merge pull-requests. I would suggest such a policy include elements of:

- minimum number of committers required for approval of a pull-request

- no pull-request to be merged by its author

I have created a page in the VIVO GitHub as a starting point for refinement: 

https://github.com/vivo-project/VIVO/wiki/Development-Processes

3. Nature of code review: This speaks directly to the quality of product we want to create and maintain. I agree with your suggestions around line-by-line review and testing... as well as exploring edge cases.

I have added this topic to next week's (Tues 1/30) Development Meeting agenda to get more ideas on the table and move towards consensus.

https://wiki.duraspace.org/display/VIVO/2018-01-30+-+VIVO+Development+IG

Thanks for kicking this off, Jim!

Andrew

--
You received this message because you are subscribed to the Google Groups "VIVO Tech" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vivo-tech+unsubscribe@googlegroups.com.
To post to this group, send email to vivo...@googlegroups.com.
Visit this group at https://groups.google.com/group/vivo-tech.
To view this discussion on the web visit https://groups.google.com/d/msgid/vivo-tech/C731B6A2-F80A-44B0-B1C0-D1E534354D05%40cornell.edu.
For more options, visit https://groups.google.com/d/optout.