Review procedure for code review

[edhartnett]

Code review is a well-proven tool for detecting defects and improving code.

To aid the discussion here's what we have now:

All code is subject to review.

Pull requests will be as small as possible and focused on one(-ish) issue.

All non-trivial pull requests are associated with a github issue. The issue is where discussion of requirements and implementation details can be worked out.

Pull requests will be left up on github for about a day. Request more time if you need it and are actively reviewing the code. (Note that pull request can also be reviewed after they are merged, if you miss one).

Jim will identify key stakeholders in chaged code and ensure they accept code changes.

Reviewers are open-minded and ready to accept improvements to the library.

Reviewers will make comments on the pull request. All comments must be resolved.

If chages are dictated, they happen on the branch, so code reviewers can see the updated code.

The pull request is only merged when all programmers agree that all issues have been resolved.

[gold2718]

Heavy PIO user (e.g., CAM history), first time poster :)
I jumped in because I saw Rob (who usually doesn't comment on PIO) jump in on #236 .
While I don't have much of an issue with code_style.txt, it seems to be serving two fairly distinct purposes: code style guidelines and code review procedures. Would it be possible to break this into two more specific documents? The code review doc would reference the code style doc as a requirement for pull requests.

[edhartnett]

Howdy Goldy! The problem is that the document is misnamed. It is a guide for contributors to PIO, so it has both a code style and code review section, and can also include any other information that helps programmers contribute to PIO. How about I change the name of the document to contributing_code.txt?

[gold2718]

While I prefer shorter, more focused documents, I'm fine with the name change.

[rljacob]

Often simply implementing the change is more efficient then discussing it beforehand (because talking about code is hard) and shows exactly what you want to do. And github makes it easy to discuss code in the context of a PR. So when you want to discuss a change with actual code, make a PR but put "RFC" in the front of the title. This means you are "requesting comment" and not saying its done. If it turns out to be done, the title can be edited to remove the "RFC".
(I've been wanting to propose this in CIME...)

[rljacob]

The PIO lead developer (Jim Edwards) should have the authority to merge a PR over other programmers objections (benevolent dictator model).

[edhartnett]

I agree that Jim should have authority to resolve disputes. This may involve merging a PR over other programmers objections, but more likely will mean weighing in on whatever issues are holding back approval. There may be several contentious issues in the review, and they may each be resolved in different ways. (Some changes kept, others kept with modifications, other changes discarded.)

I have no objection to any contributor bringing forward code suggestions in any form, including pull requests, github issues, emails, or verbally. All are welcome. Programmers that wish to explore ideas in the media of code must accept that not all ideas will be accepted. Sometimes that means you will write code that does not get used (but that might still be a very productive way to work - just be advised).

Here's how I handle it.

I have made many large changes to the code, but I have usually talked them over with Jim first, at our meetings, or in email, just to make sure we are on the same page. I almost always put up a github issue as well. Then I start with a small subset of the work to demonstrate the approach. After he approves that PR, I will do another (or more than one), to apply the changes more widely.

Sometimes that means I work a little harder to develop a solution in parallel to existing code. After my parallel solution has been approved in a small subset of cases, I can apply it more widely in the code, and remove the older implementation of that function. (This is happening right now, with the CheckMPIReturn function being replaced with check_mpi, which handles errors more correctly.)

If anyone has reservations about my PR, I don't hesitate to take it down. I usually will then immediately submit another with whatever changes were non-controversial. That PR will quickly get approved and I will then move on to the more controversial changes, taking another stab at them, with previous objections in mind. The game is to come up with a set of changes that accomplish my goals, which trigger no objections.

By the time Jim sees my code in review, he knows what is coming, we agree on the approach, and all that's left is to spot mistakes in the implementation. (And he sometimes does, which is super helpful.)

So it's not so much about whether or not there is a rule about having a github issue. The important thing is the code review goes smoothly, and work does not have to be redone, if all interested programmers are kept on-board as the solution is developed and applied.

Large amounts of non-controversial changes can be accomplished very quickly, but even one line of controversy can cause a lot of discussion (and it should).

As a programmer I try to reduce controversy in my PRs by:

• never have unneeded code
• using consistent variable naming
• neat code, formatted consistently (mostly done automatically)
• comments and required doxygen documentation
• follow the idioms of the other functions (or suggest a change in idiom to the team)

If I want to do something controversial, I try to keep it separate. I have not always been good at that, but, responding from request from other members of the team, I have been trying to make my PRs very small and self-contained, with a minimum going on. So you can review those that interest you, and skip the ones that don't.

Another thing I have been trying to do is separate changes to documentation, test code, and library code. Sometimes I have to change all of them, but if I only have changes to documentation, or test code, I will say so in the PR.

[gold2718]

Is there any testing requirement before submitting a pull request with code changes? I see an item about compiler warnings but no indication of under what conditions the code has to be compiled. That would be helpful (to me at least).

[gold2718]

I think there is a typo on line 109 (chaged).

[edhartnett]

I guess everyone has had a chance to comment, so I will make the changes to the document and submit them in a PR.

It's not clear to me if these review procedures are actually in effect or not, or if they apply to all programmers. However I will continue to abide by them with my pull requests. I urge the other project programmers to do the same.

[jedwards4b]

Thanks for putting this together.