OpenJFX code review policies, etc.

[Phil Race]

One other point .. something I mentioned off line to Kevin but he did 
not so far bring up here,
is that when counting the number of reviewers on a fix, we must require 
at least one Reviewer,
with a capital "R", but to make up the total of 2 reviewers, it is 
sufficient to have one other person
who only has committer status .. or even author status (maybe), be the 
second reviewer.
We've used this policy on the JDK AWT/2D/Swing stack for a long time. It 
not only frees up the
scarce "R"eviewers, but it helps train new ones :-), whilst still 
ensuring at least two sets of eye balls
were on it.

-phil.

On 05/24/2018 10:36 AM, Kevin Rushforth wrote:
> Phil pointed out one glaring typo in the summary and also a couple 
> things in the details that could be clarified.
>
>> The short version of the proposal is:
>> ...
>> 2. Revised code review policies for different types of changes: 
>> simple, low-impact fixes (1 Reviewer); higher-impact fixes (2 
>> Reviewers + allow time for others to chime in); Features / API 
>> changes (CSR approval, including approval by a "lead", plus 3 
>> Reviewers for the code)
>>
>
> That last been should be: CSR approval, including approval by a 
> "lead", plus *2* Reviewers for the code. I had it right in the 
> details, but made a typo in the short version. While some reviews 
> might have more than 2, it was certainly not my intent to mandate it.
>
>> A. Low-impact bug fixes. ...
>> One reviewer is sufficient to accept such changes. As a courtesy, and 
>> to avoid changes which later might need to be backed out, if you 
>> think there might be some concern or objection to the change, please 
>> give sufficient time for folks who might be in other time zones the 
>> chance to take a look. This should be left up to the judgment of the 
>> reviewer who approves it as well as the contributor.
>
> To clarify, the intent is to avoid pushing changes that might be 
> controversial, and not to mandate unnecessary delay for truly simple 
> fixes (e.g., fixing a build break). Reviewers and Committers are 
> expected to use their best judgment here.
>
>> C. New features / API additions. This includes behavioral changes, 
>> additions to the fxml or css spec, etc.
>>
>> ... a New Feature, API addition, or behavioral change must be 
>> reviewed / approved by a "lead". Currently this is either myself or 
>> Johan Vos as indicated above.
>>
>> I also propose that we continue to use the CSR process [3] to track 
>> such changes. The CSR chair has indicated that he is willing to track 
>> JavaFX compatibility changes even though FX is no longer a part of 
>> the JDK.
>
> The "approval by lead" means approving the API / feature change via 
> the CSR. A "lead" often will be one of the code reviewers as well, but 
> need not be as long as they approve the API change itself via the CSR.
>
> -- Kevin
>
>
> On 5/23/2018 3:16 PM, Kevin Rushforth wrote:
>> To: OpenJFX Developers
>>
>> As I mentioned in a message last week [1] I would like to restart the 
>> discussion we started a few months ago [2] around making it easier to 
>> contribute code to OpenJFX. To this end, I like to make some concrete 
>> proposals around code review / API review policies.
>>
>> Before getting to the details, I would like to acknowledge Gluon's 
>> contributions to the OpenJFX project, specifically those of Johan 
>> Vos. I am pleased to announce an expanded role for Johan Vos in the 
>> OpenJFX project. I would like to announce that starting now, Johan is 
>> effectively a co-lead for the purposes of setting direction, and 
>> approving new features for the Project.
>>
>> The short version of the proposal is:
>>
>> 1. Formalize the concept of Reviewers with an initial list of 
>> Reviewers and a defined criteria for adding additional Reviewers.
>>
>> 2. Revised code review policies for different types of changes: 
>> simple, low-impact fixes (1 Reviewer); higher-impact fixes (2 
>> Reviewers + allow time for others to chime in); Features / API 
>> changes (CSR approval, including approval by a "lead", plus 3 
>> Reviewers for the code)
>>
>> 3. Streamlining reviews of changes developed in the GitHub sandbox: 
>> provided that the review policy is followed to before a PR is merged 
>> into the develop branch in GitHub, a fast-track review can happen 
>> pointing to the changeset that was merged and the PR, which has 
>> review comments.
>>
>> Details follow.
>>
>> Quoting from my earlier message:
>>
>>> "Code reviews are important to maintain high-quality contributions, 
>>> but we recognize that not every type of change needs the same level 
>>> of review. Without lowering our standards of quality, we want to 
>>> make it easier to get low-impact changes (simple bug fixes) accepted."
>>
>> To that end, I propose the following policies. Many of these will 
>> involve judgment calls, especially when it comes to deciding whether 
>> a fix is low impact vs. high-impact. I think that's OK. It doesn't 
>> have to be perfect.
>>
>> Recommendations
>>
>> 1. I recommend that we formalize the concept of Reviewers, using the 
>> OpenJDK Reviewer role for the OpenJFX Project.
>>
>> A. I will provide an initial list of reviewers to the registrar based 
>> on past contributions, and also recognizing Committers who have 
>> become experts in their area. This is the only time we will have such 
>> latitude as the OpenJDK Bylaws specify the policy we need to follow 
>> for nominating and voting upon additional Reviewers.
>>
>> B. We need to set formal guidelines for becoming a Reviewer. The JDK 
>> uses a threshold of 32 significant contributions. While we don't want 
>> to relax it too much, one thing I have been discussing informally 
>> with a few people is that a Committer with, say, 24 commits, who 
>> regularly participates in reviews, offering good feedback, might be 
>> just a good a reviewer (maybe even better) than someone with 32 
>> commits who rarely, if ever, provides feedback on proposed bug fixes. 
>> I'm open for suggestions here.
>>
>> One thing I'd like to add is that we expect Reviewers to feel 
>> responsible not just for their piece, but for the quality of the 
>> JavaFX library as a whole. I might work with some folks at Gluon and 
>> here at Oracle to draft a set of expectations for reviewers.
>>
>>
>> 2. Code review policies
>>
>> All code reviews must be posted on the openjfx-dev mailing list -- 
>> even simple fixes. I propose that we have the following code review 
>> policies for different types of changes. I also note that if there is 
>> disagreement as to whether a fix is low-impact or high-impact, then 
>> it is considered high-impact. In other words we will always err on 
>> the side of quality by "rounding up" to the next higher category. The 
>> contributor can say whether they think something is low-impact or 
>> high-impact, but It is up to a Reviewer to initially decide this.
>>
>> A. Low-impact bug fixes. These are typically isolated bug fixes with 
>> little or no impact beyond fixing the bug in question; included in 
>> this category are test fixes (including new tests), doc fixes, and 
>> fixes to sample applications (including new samples).
>>
>> One reviewer is sufficient to accept such changes. As a courtesy, and 
>> to avoid changes which later might need to be backed out, if you 
>> think there might be some concern or objection to the change, please 
>> give sufficient time for folks who might be in other time zones the 
>> chance to take a look. This should be left up to the judgment of the 
>> reviewer who approves it as well as the contributor.
>>
>> B. Higher impact bug fixes or RFEs. These include changes to the 
>> implementation that potentially have a performance or behavioral 
>> impact, or are otherwise broad in scope. Some larger bug fixes will 
>> fall into this category, as will fixes in high-risk areas (e.g., CSS).
>>
>> Two reviewers must approve to accept such changes. Additionally, the 
>> review should allow sufficient time for folks who might be in other 
>> time zones the chance to review if they have concerns.
>>
>> C. New features / API additions. This includes behavioral changes, 
>> additions to the fxml or css spec, etc.
>>
>> Feature requests come with a responsibility beyond just saying "here 
>> is the code for this cool new feature, please take it". There are 
>> many factors to consider for even small features. Larger features 
>> will need a significant contribution in terms of API design, coding, 
>> testing, maintainability, etc.
>>
>> To ensure that new features are consistent with the rest of the API 
>> and the desired direction of the Project, I propose that a New 
>> Feature, API addition, or behavioral change must be reviewed / 
>> approved by a "lead". Currently this is either myself or Johan Vos as 
>> indicated above.
>>
>> I also propose that we continue to use the CSR process [3] to track 
>> such changes. The CSR chair has indicated that he is willing to track 
>> JavaFX compatibility changes even though FX is no longer a part of 
>> the JDK.
>>
>> For the review of the implementation, I propose that we use the same 
>> "two reviewer" standard for the code changes as category B.
>>
>>
>> 3. Streamlining the review process for changes developed on GitHub
>>
>> A fix that was developed as pull-requests (PRs) on GitHub is eligible 
>> for a fast-track review, if:
>>
>> A. The PR was squashed / merged into the develop branch as a single 
>> changeset
>> B. No follow-on changesets were merged into develop as part of that 
>> same fix
>> C. The changeset is "whitespace clean" -- meaning that you have run 
>> 'tools/scripts/checkWhiteSpace' on the final changeset (we might want 
>> to add this to the CI build).
>> and
>> D. All code review policies outlined above in #2 were followed prior 
>> to the PR being approved and merged into the develop branch on 
>> GitHub. This includes sending email to openjfx-dev when you first 
>> make a PR that you intend to have merged into the develop branch to 
>> give other reviewers who may not be watching all PRs a chance to 
>> comment before it is merged.
>>
>> A "fast-track" review is a quick sanity check before the change is 
>> committed and pushed to the jfx-dev repo on hg.openjdk.java.net. This 
>> fast track review just needs to point to the GitHub changeset that 
>> was merged and to the PR, which will have any review comments. If 
>> there are no compelling reasons why the PR can't be pushed to 
>> jfx-dev, then it can be pushed.
>>
>>
>> Please let me know your thoughts on the above proposals.
>>
>> Thank you all for being a part of this community.
>>
>> -- Kevin Rushforth, OpenJFX Project Lead
>>
>> [1] 
>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-May/021867.html
>>
>> [2] 
>> http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-February/021335.html
>>
>> [3] https://wiki.openjdk.java.net/display/csr/Main
>>
>