8332313: Update code review guidelines #1455
Conversation
Initial version of Code Review Guidelines copied from the Wiki and converted to markdown format. No substantive changes have been made to the guidelines, so this is a baseline representing the current guidelines as documented on the Wiki.
Add sections on: -- Guidelines for reviewing of a PR -- Before you integrate or sponsor a PR
|
👋 Welcome back kcr! A progress list of the required criteria for merging this PR into |
|
@kevinrushforth This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 3 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
|
Reviewers: @johanvos @andy-goryachev-oracle @arapte @prrace I'd like at least 3 "R"eviewers to review this. Committers and Authors are also welcome to provide feedback. /reviewers 3 reviewers |
|
@kevinrushforth |
CONTRIBUTING.md : Highlight things to check before integration (point to README-code-reviews.md for details) README-code-reviews.md : Move details of checking commit message to here
Webrevs
|
| @@ -46,7 +46,7 @@ If you are a first time contributor to OpenJFX, welcome! Please do the following | |||
|
|
|||
| * Read the code review policies | |||
|
|
|||
| Please read the entire section below on how to submit a pull request, as well as the [OpenJFX Code Review Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews). If this is a feature request, please note the additional requirements and expectations in the [New features / API additions](#new-features--api-additions) section at the end of this guide. | |||
| Please read the entire section below on how to submit a pull request, as well as the [Code Review Policies](README-code-reviews.md) page. If this is a feature request, please note the additional requirements and expectations in the [New features / API additions](#new-features--api-additions) section at the end of this guide. | |||
|
|
|||
| * File a bug in JBS for every pull request | |||
|
|
|||
There was a problem hiding this comment.
for line 53, would it make more sense to use this link
https://bugs.openjdk.org/projects/JDK/issues/
?
There was a problem hiding this comment.
Whether or not that might be a better link, it's unrelated to this PR. The scope of this PR is limited to guidelines surrounding reviewing, integrating, and sponsoring PRs.
README-code-reviews.md
Outdated
| The OpenJFX Project is guided by the Project Leads and Reviewers. | ||
|
|
||
| __Project Co-Lead__: Kevin Rushforth (kcr) <br> | ||
| __Project Co-Lead__: Johan Vos (jvos) |
There was a problem hiding this comment.
There are two sets of ids - one for OpenJFX/JBS and one for Github. This might be confusing sometimes, should we list both?
There was a problem hiding this comment.
Not sure what you mean, but it doesn't seem related to this PR.
There was a problem hiding this comment.
He means people reading reviews are usually on github and your github ID is kevinrusforth,
so they might be confused by "kcr" which is your openjdk id.
Since the context is identifying OpenJDK Project leads, I think the OpenJDK ID is the right and only one to include here. At most you could clarify like (OpenJDK ID - kcr)
There was a problem hiding this comment.
exactly my point. when the majority of time is spent in the context of github (PR review etc) it might be confusing when one types @jvos and it's not found, or when within a PR there are references to both jvos and johanvos
(see the very first comment in #1388 )
There was a problem hiding this comment.
Ah, I misunderstood Andy's comment. Yes, I'll update this.
|
|
||
| Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer: | ||
|
|
||
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue |
There was a problem hiding this comment.
Perhaps this should be noted as a required step for the PR author (maybe as a template text in the PR description?).
There was a problem hiding this comment.
I like the idea of adding some additional information on "here's what makes a good PR" to the CONTRIBUTING guidelines. I'll either take a stab at this or else file a follow-on issue.
| Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer: | ||
|
|
||
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Focus first on substantive comments rather than stylistic comments |
There was a problem hiding this comment.
this is not as important as the next two.
There was a problem hiding this comment.
Agreed. I'll move this bullet down.
README-code-reviews.md
Outdated
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Focus first on substantive comments rather than stylistic comments | ||
| * Consider the risk of regression | ||
| * Consider any compatibility concerns |
There was a problem hiding this comment.
regression and compatibility risks are probably the most important aspects of the code review from the "R"eviewer's perspective. Perhaps this should be emphasized somehow?
There was a problem hiding this comment.
That's a good idea.
There was a problem hiding this comment.
In the ideal world where we have tons of regression and compatibility tests, I would agree. Unfortunately, we are totally not there yet. Compared to other projects, the quality of tests in OpenJFX is good, but given the multitude of usage scenario's in client development, we would need much, much more tests before we can rely on them to detect regression.
Therefore, the #1 point imho is that both the author (as Andy commented) and the reviewer have a real good understanding on what is happening. Every single change in a PR should be understood. The most dangerous things are "I don't understand why, but it seems to fail before and succeed after the patch".
There was a problem hiding this comment.
Yes, this a good point. I left the "make sure you understand..." bullet as the first, and moved the "Focus first on substantive comments" below the regression and compatibility bullets.
README-code-reviews.md
Outdated
| * All substantive feedback has been addressed, especially any objections from one with a Reviewer role. | ||
| * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this. | ||
| * The PR has been "rfr" (as indicated by Skara) for at least 1 business day (24 hours), not including weekends / or major holidays. This is to allow sufficient time for those reviewers who might be in other time zones the chance to review if they have concerns. This is measured from the time that Skara has most recently added the "rfr" label (for example, for a PR that was previously in Draft mode, wait for at least 24 hours after the PR has been taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) a Reviewer might give the OK to integrate without waiting for 24 hours. | ||
| * Double-check the commit message before you integrate. Skara will list it near the top of your PR. |
There was a problem hiding this comment.
is this needed? SKARA makes sure that it matches the JBS title.
|
|
||
| #### 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 most new tests), doc fixes, and fixes to sample applications (including most new samples). |
There was a problem hiding this comment.
I think the main criteria is regression and compatibility impact. Should we expand upon that here?
There was a problem hiding this comment.
That seems like a good idea.
|
|
||
| A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take. | ||
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. |
There was a problem hiding this comment.
this paragraph feels extremely nebulous.
a minor issue is that CSR cannot be written in full detail before the draft PR is created (a PoS is written),
a larger issue is that a decision to implement a feature might need a better process - is one lead approval sufficient? what happens when leads disagree?
There was a problem hiding this comment.
This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.
|
|
||
| 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. | ||
|
|
||
| A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take. |
There was a problem hiding this comment.
are there any special rules to gauging the consensus? what happens when one party in a discussion stops responding?
There was a problem hiding this comment.
This is out of scope for this PR (this section is unchanged). It might be something to consider addressing in a follow-on enhancement, although I'm not sure I want to make it any more specific.
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. | ||
|
|
||
| The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel. |
There was a problem hiding this comment.
it is not clear (at least to me) the required order in respect to CSR approval. for a new feature, does the CSR need to be approved before the work can be started, or the draft PR is published, or an PR enters its "rfr" stage, or just before the integration?
Or perhaps it should be clarified that the CSR is just a human-readable, specially formatted specification of the change, and it is an integral part of the new feature that needs to be approved before the feature can be integrated?
There was a problem hiding this comment.
This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.
There was a problem hiding this comment.
It should be made clear (somewhere, at some point) that it would be rare for a CSR to be completed and approved before starting work on the implementation because the spec. almost always evolves as the fix is developed.
There was a problem hiding this comment.
Yes, that is a good point. I'll file a follow-up to do that (since this section is otherwise unmodified from the current guidelines on the Wiki).
README-code-reviews.md
Outdated
|
|
||
| All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request. | ||
|
|
||
| All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. 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 confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas). |
There was a problem hiding this comment.
I think it worth noting that in skara syntax that isn't two people with the reviewer role.
And tell people what to use if that is what they intend - eg if I have it right
/reviewers reviewers 2
There was a problem hiding this comment.
It's /reviewers 2 reviewers, so I'll add that as an example and clarify that /reviewers 2 is 2 with at least one reviewer.
There was a problem hiding this comment.
Fixed (both the example Phil asked for and the typo Nir noted)
README-code-reviews.md
Outdated
|
|
||
| All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request. | ||
|
|
||
| All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. 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 confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas). |
There was a problem hiding this comment.
About requesting reviews. I think that only some people can request reviews through GitHub, I never managed to do it on this repo, probably a matter of permissions. Might worth clarifying how this works.
There was a problem hiding this comment.
It's better to just add comments @ mentioning who you want to do the review, since there isn't a way for most people to request a review from others.
README-code-reviews.md
Outdated
| * Consider the risk of regression | ||
| * Consider any compatibility concerns | ||
| * Check whether there is an automated test; if not, ask for one, if it is feasible | ||
| * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows |
There was a problem hiding this comment.
These tests sometimes fail and we integrate anyway. I noticed that sometimes they need a few iterations to succeed. Are we really dependent on them?
Edit: currently the Linux test is failing, and this PR can't be the reason.
There was a problem hiding this comment.
or is it? :-)
There was a problem hiding this comment.
Isn't this automatic? Seems weird you could integrate without these passing.
There was a problem hiding this comment.
or is it? :-)
Looks to me like it is...
There was a problem hiding this comment.
A passing GHA test run is neither necessary nor sufficient. It is an interesting data point. A PR can be integrated with a failing run as long as we understand why it failed. It is something for a reviewer to check and ask about if they suspect it points to a real problem. I'll reword this a little bit to make it clear. I'm sort of glad that GitHub / Azure chose today to break it. It is a great illustration of why we don't want it to be blocking.
Btw, I filed https://bugs.openjdk.org/browse/JDK-8332328 to track the GHA failure.
There was a problem hiding this comment.
I updated the wording slightly
README-code-reviews.md
Outdated
| * Consider any compatibility concerns | ||
| * Check whether there is an automated test; if not, ask for one, if it is feasible | ||
| * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows | ||
| * If the PR source branch hasn't synced up from master in a long time, or if there is an upstream commit not in the source branch that might interfere with the PR, ask the PR author to merge the latest upstream master. |
There was a problem hiding this comment.
This is the only bullet point with a period at the end.
| * If you want to indicate your approval, but still feel additional reviewers are needed, you may increase the number of reviewers (e.g., from 2 to 3) | ||
| * If you want an area expert to review a PR, indicate this in a comment of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can indicate whether or not they plan to review it | ||
| * If you want to ensure that you have the opportunity to review this PR yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this PR`, optionally add any concerns you might have | ||
|
|
There was a problem hiding this comment.
I would add that it's a good idea to search for JBS issues similar/linked to the one being targeted. There are many unfound duplicates in JBS that arise only when searching, or at least closely linked ones (problems in TableTreeView often have "pseudo-duplicates" for TreeView and TableView, for example).
I would explain (or link to where it is explained) when to close an issue a a duplicated vs. when to use the /add Skara command to bundle issues into the same fix.
We might also find that the targeted issue is a regression of another issue. This is somewhat mentioned in the bullet point below "Consider the risk of regression".
There was a problem hiding this comment.
Second this, although I think the bulk of the work should fall on the author of the PR.
There was a problem hiding this comment.
This would probably be better in the CONTRIBUTING guidelines (for PR authors). Maybe as a follow-on, but I'll give it some thought.
| * If you want to indicate your approval, but still feel additional reviewers are needed, you may increase the number of reviewers (e.g., from 2 to 3) | ||
| * If you want an area expert to review a PR, indicate this in a comment of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can indicate whether or not they plan to review it | ||
| * If you want to ensure that you have the opportunity to review this PR yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this PR`, optionally add any concerns you might have | ||
|
|
There was a problem hiding this comment.
Another thing to look out for is the targeting branch during rampdown. Should they all target master and then be backported as needed, or can some target the rampdown branch?
There was a problem hiding this comment.
Good idea. I'll add the bit about checking the target branch -- if the PR isn't a backport it should almost always target the master branch (there might be very rare exception where a bug is specific to the stabilization branch).
README-code-reviews.md
Outdated
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Focus first on substantive comments rather than stylistic comments | ||
| * Consider the risk of regression | ||
| * Consider any compatibility concerns |
There was a problem hiding this comment.
This might be included in this point already, but one thing I sometimes miss is the inadvertent introduction of new API (that will have to be deprecated if missed). This can happen with protected methods, default public constructors (esp. if a non-API constructor is removed), or if a class is moved from a non-exported to an exported package (something that you can't see by looking at the area you're reviewing, you need to check the module-info, or more practically, look at the package names).
There was a problem hiding this comment.
I was wondering if there ought to be an unofficial checklist for things to look for?
As new people become "R"eviewers, I think there is a value in accumulating collective wisdom in a checklist. I like checklists.
There was a problem hiding this comment.
...inadvertent introduction of new API (that will have to be deprecated if missed)
I think this is worth mentioning.
I was wondering if there ought to be an unofficial checklist for things to look for?
That's sort of what this section is meant to be. As long as it doesn't get too verbose we could add additional things to look for.
There was a problem hiding this comment.
...inadvertent introduction of new API (that will have to be deprecated if missed)
I think this is worth mentioning.
Fixed.
README-code-reviews.md
Outdated
|
|
||
| * Determine whether this needs 2 reviewers and whether it needs a CSR; issue the `/reviewers 2` or `/csr` command as needed | ||
| * If you want to indicate your approval, but still feel additional reviewers are needed, you may increase the number of reviewers (e.g., from 2 to 3) | ||
| * If you want an area expert to review a PR, indicate this in a comment of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can indicate whether or not they plan to review it |
There was a problem hiding this comment.
Should a list of experts per area be made available somewhere? The Wiki has an old "code ownership" table that is out of date. Usually you get to know the experts only after they have reviewed your code a couple of times.
There was a problem hiding this comment.
That might be something to consider as a follow-on.
|
|
||
| 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. | ||
|
|
||
| A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take. |
There was a problem hiding this comment.
This is out of scope for this PR (this section is unchanged). It might be something to consider addressing in a follow-on enhancement, although I'm not sure I want to make it any more specific.
|
|
||
| A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take. | ||
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. |
There was a problem hiding this comment.
This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. | ||
|
|
||
| The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel. |
There was a problem hiding this comment.
This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.
|
|
||
| Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer: | ||
|
|
||
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue |
There was a problem hiding this comment.
I like the idea of adding some additional information on "here's what makes a good PR" to the CONTRIBUTING guidelines. I'll either take a stab at this or else file a follow-on issue.
| Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer: | ||
|
|
||
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Focus first on substantive comments rather than stylistic comments |
There was a problem hiding this comment.
Agreed. I'll move this bullet down.
README-code-reviews.md
Outdated
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Focus first on substantive comments rather than stylistic comments | ||
| * Consider the risk of regression | ||
| * Consider any compatibility concerns |
There was a problem hiding this comment.
That's a good idea.
|
|
||
| #### 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 most new tests), doc fixes, and fixes to sample applications (including most new samples). |
There was a problem hiding this comment.
That seems like a good idea.
README-code-reviews.md
Outdated
| * All substantive feedback has been addressed, especially any objections from one with a Reviewer role. | ||
| * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this. | ||
| * The PR has been "rfr" (as indicated by Skara) for at least 1 business day (24 hours), not including weekends or major holidays. This is to allow sufficient time for those reviewers who might be in other time zones the chance to review if they have concerns. This is measured from the time that Skara has most recently added the "rfr" label (for example, for a PR that was previously in Draft mode, wait for at least 24 hours after the PR has been taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) a Reviewer might give the OK to integrate without waiting for 24 hours. | ||
| * Verify the commit message. The Skara tooling adds a comment near the top of the PR with the commit message that will be used. You can add a summary to the commit message with the `/summary` command. You can add additional contributors with the `/contributor` command. Commands are issued by adding a comment to the PR that starts with a slash `/` character. |
There was a problem hiding this comment.
The last sentence feels a bit out of place, as several places earlier in this document already mention slash commands.
There was a problem hiding this comment.
I'll remove it (I moved it from the CONTRIBUTING doc, and it does seem out of place).
Actually as someone else commented, Skara will verify that the PR title and JBS bug match, so mostly this is about adding a summary and/or contributors if you want to.
README-code-reviews.md
Outdated
| * Consider the risk of regression | ||
| * Consider any compatibility concerns | ||
| * Check whether there is an automated test; if not, ask for one, if it is feasible | ||
| * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows |
There was a problem hiding this comment.
Isn't this automatic? Seems weird you could integrate without these passing.
CONTRIBUTING.md
Outdated
| @@ -229,6 +230,8 @@ Please also follow these formatting guidelines: | |||
| * Wildcard imports – for example, `import java.util.*;` – are forbidden and may cause the build to fail. Please attempt to configure your IDE so it doesn't generate wildcard imports. An exception to this rule is that wildcard static imports in test classes are allowed, for example, `import static org.junit.Assert.*;`. | |||
| * Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so. | |||
|
|
|||
| New code should be formatted consistently in accordance with the above guidelines. However, please do not reformat existing code as part of a bug fix. The makes more changes for code reviewers to track and review, and can lead to merge conflicts. If you want to reformat a class, do that in a separate pull request (which will need its own unique JBS bug ID). | |||
There was a problem hiding this comment.
"The makes more changes" ? I think you mean "This" not "The"
I'm not sure about the last sentence, it seems to encourage reformatting fixes which are just noise most of the time.
There was a problem hiding this comment.
Yeah, that was a typo (which I didn't notice when copying the block from the other doc). I'll fix it. And I agree with your concern, so I'll remove the last sentence.
There was a problem hiding this comment.
I agree with the concern, but I still think it's much better to encourage developers to do formatting in a separate issue (or not at all) with all the required administration, than to sneak in a formatting change in a PR that has nothing to do with formatting. I prefer the noise to be completely isolated, so that it can be ignored easily, rather than being distracted by it in a PR.
There was a problem hiding this comment.
I think this conflicts a bit with the previous statement ("don't worry too much about import order"). In my experience, when merging complex changes, import order changes are often the cause of conflicts. We also found that with many different committer preferences, the whole import block could change from one commit to the next, which will lead to a conflict if such a file is part of a backport.
We solved this by simply mandating a single order for imports (alphabetical, no gaps, no star imports, followed by separate block for static imports with the same rules) and made the build fail when it was violated. The number of conflicts went down dramatically. This does require a one time import fix of the code base, which will also be a source of conflicts, but at least it will be in the usual area (imports) and should solve the problem once and for all.
There was a problem hiding this comment.
I removed the bit encouraging reformatting PRs (but left the "don't reformat existing code..." bit.
As for the import order, this came up recently in Andy's PRs to remove unnecessary imports. I'll file a follow-on issue to evaluate whether we want to change our current policy.
README-code-reviews.md
Outdated
|
|
||
| ### Reviewers | ||
|
|
||
| The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on the OpenJDK Census. |
There was a problem hiding this comment.
We use ".org" now, not ".java.net"
There was a problem hiding this comment.
Yes, I missed this. I'll update.
README-code-reviews.md
Outdated
|
|
||
| ### 1. The Reviewer role for the OpenJFX Project | ||
|
|
||
| We define a formal "Reviewer" role, similar to the JDK project. A [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for reviewing code changes and helping to determine whether a change is suitable for including into OpenJFX. We expect Reviewers to feel responsible not just for their piece, but for the quality of the JavaFX library as a whole. In other words, the role of Reviewer is one of stewardship. See the following section for what constitutes a good review. |
There was a problem hiding this comment.
(https://openjdk.java.net/census#openjfx)
.org please
BTW these very long source lines make it awkward to precisely identify the text I'm commenting on.
README-code-reviews.md
Outdated
|
|
||
| All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request. | ||
|
|
||
| All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. 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 confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas). |
There was a problem hiding this comment.
I think it worth noting that in skara syntax that isn't two people with the reviewer role.
And tell people what to use if that is what they intend - eg if I have it right
/reviewers reviewers 2
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. | ||
|
|
||
| The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel. |
There was a problem hiding this comment.
It should be made clear (somewhere, at some point) that it would be rare for a CSR to be completed and approved before starting work on the implementation because the spec. almost always evolves as the fix is developed.
README-code-reviews.md
Outdated
| Skara will mark a PR as "ready" once the minimum number of reviewers have reviewed it. Before you integrate or sponsor the PR, ensure that the following have been done: | ||
|
|
||
| * All substantive feedback has been addressed, especially any objections from one with a Reviewer role. | ||
| * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this. |
There was a problem hiding this comment.
One thing to add here (or hereabouts) is that if someone has commented on your review and requested changes that in almost all cases you should expect that they will want to return to review the results. So DO NOT push without letting earlier reviewers who made substantive comments re-review.
There was a problem hiding this comment.
I'll add something about this.
CONTRIBUTING.md
Outdated
| @@ -229,6 +230,8 @@ Please also follow these formatting guidelines: | |||
| * Wildcard imports – for example, `import java.util.*;` – are forbidden and may cause the build to fail. Please attempt to configure your IDE so it doesn't generate wildcard imports. An exception to this rule is that wildcard static imports in test classes are allowed, for example, `import static org.junit.Assert.*;`. | |||
| * Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so. | |||
|
|
|||
| New code should be formatted consistently in accordance with the above guidelines. However, please do not reformat existing code as part of a bug fix. The makes more changes for code reviewers to track and review, and can lead to merge conflicts. If you want to reformat a class, do that in a separate pull request (which will need its own unique JBS bug ID). | |||
There was a problem hiding this comment.
Yeah, that was a typo (which I didn't notice when copying the block from the other doc). I'll fix it. And I agree with your concern, so I'll remove the last sentence.
README-code-reviews.md
Outdated
| The OpenJFX Project is guided by the Project Leads and Reviewers. | ||
|
|
||
| __Project Co-Lead__: Kevin Rushforth (kcr) <br> | ||
| __Project Co-Lead__: Johan Vos (jvos) |
There was a problem hiding this comment.
Ah, I misunderstood Andy's comment. Yes, I'll update this.
README-code-reviews.md
Outdated
|
|
||
| ### Reviewers | ||
|
|
||
| The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on the OpenJDK Census. |
There was a problem hiding this comment.
Yes, I missed this. I'll update.
README-code-reviews.md
Outdated
|
|
||
| ### 1. The Reviewer role for the OpenJFX Project | ||
|
|
||
| We define a formal "Reviewer" role, similar to the JDK project. A [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for reviewing code changes and helping to determine whether a change is suitable for including into OpenJFX. We expect Reviewers to feel responsible not just for their piece, but for the quality of the JavaFX library as a whole. In other words, the role of Reviewer is one of stewardship. See the following section for what constitutes a good review. |
README-code-reviews.md
Outdated
|
|
||
| All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request. | ||
|
|
||
| All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. 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 confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas). |
There was a problem hiding this comment.
It's /reviewers 2 reviewers, so I'll add that as an example and clarify that /reviewers 2 is 2 with at least one reviewer.
README-code-reviews.md
Outdated
| * Consider the risk of regression | ||
| * Consider any compatibility concerns | ||
| * Check whether there is an automated test; if not, ask for one, if it is feasible | ||
| * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows |
There was a problem hiding this comment.
A passing GHA test run is neither necessary nor sufficient. It is an interesting data point. A PR can be integrated with a failing run as long as we understand why it failed. It is something for a reviewer to check and ask about if they suspect it points to a real problem. I'll reword this a little bit to make it clear. I'm sort of glad that GitHub / Azure chose today to break it. It is a great illustration of why we don't want it to be blocking.
Btw, I filed https://bugs.openjdk.org/browse/JDK-8332328 to track the GHA failure.
README-code-reviews.md
Outdated
| * Consider any compatibility concerns | ||
| * Check whether there is an automated test; if not, ask for one, if it is feasible | ||
| * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows | ||
| * If the PR source branch hasn't synced up from master in a long time, or if there is an upstream commit not in the source branch that might interfere with the PR, ask the PR author to merge the latest upstream master. |
README-code-reviews.md
Outdated
| Skara will mark a PR as "ready" once the minimum number of reviewers have reviewed it. Before you integrate or sponsor the PR, ensure that the following have been done: | ||
|
|
||
| * All substantive feedback has been addressed, especially any objections from one with a Reviewer role. | ||
| * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this. |
There was a problem hiding this comment.
I'll add something about this.
README-code-reviews.md
Outdated
| * All substantive feedback has been addressed, especially any objections from one with a Reviewer role. | ||
| * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this. | ||
| * The PR has been "rfr" (as indicated by Skara) for at least 1 business day (24 hours), not including weekends or major holidays. This is to allow sufficient time for those reviewers who might be in other time zones the chance to review if they have concerns. This is measured from the time that Skara has most recently added the "rfr" label (for example, for a PR that was previously in Draft mode, wait for at least 24 hours after the PR has been taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) a Reviewer might give the OK to integrate without waiting for 24 hours. | ||
| * Verify the commit message. The Skara tooling adds a comment near the top of the PR with the commit message that will be used. You can add a summary to the commit message with the `/summary` command. You can add additional contributors with the `/contributor` command. Commands are issued by adding a comment to the PR that starts with a slash `/` character. |
There was a problem hiding this comment.
I'll remove it (I moved it from the CONTRIBUTING doc, and it does seem out of place).
Actually as someone else commented, Skara will verify that the PR title and JBS bug match, so mostly this is about adding a summary and/or contributors if you want to.
|
|
||
| To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above. | ||
|
|
||
| The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel. |
There was a problem hiding this comment.
Yes, that is a good point. I'll file a follow-up to do that (since this section is otherwise unmodified from the current guidelines on the Wiki).
1. Add note about looking for new public API 2. Move the "focus on substantive comments" bullet down a couple items in the list
|
I addressed most of the feedback. There are still one or two more items I might update for this PR, but I'll capture the rest in follow-up issues. |
README-code-reviews.md
Outdated
|
|
||
| Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer: | ||
|
|
||
| * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue | ||
| * Carefully consider the risk of regression | ||
| * Carefully consider any compatibility concerns | ||
| * Check whether it adds any new public or protected API, even implicitly (such as a public method that overrides a protected method, or a class that is moved from a non-exported to an exported package); if it does, indicate that it needs a CSR |
There was a problem hiding this comment.
I think that if it looks like an oversight (the author forgot about the default constructor), it's better to indicate that than to indicate that the PR needs a CSR. Maybe something like:
"if it does and it doesn't look like an oversight, indicate that it needs a CSR"
There was a problem hiding this comment.
We have by now cleaned up our public API to avoid classes with an implicit no-arg constructor, so the only way this situation could arise in the future is if someone adds a new public class, which needs a CSR anyway.
I guess it could also happen if someone proposed to delete the no-arg constructor, but we don't allow deleting of non-terminally-deprecated API anyway. Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"?
Somewhat related, these guidelines don't address what to look for when reviewing new API; perhaps a follow-on issue.
There was a problem hiding this comment.
Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"?
This looks like a good idea.
|
I filed the following follow-on bug to address and pending items: JDK-8332642: Additional improvements to contribution and code review guidelines I think I have addressed all of the feedback so far, either by updating the docs or by adding an item to JDK-8332642. |
|
@johanvos Do you want to review this? |
|
/integrate |
|
Going to push as commit 94aa2b6.
Your commit was automatically rebased without conflicts. |
|
@kevinrushforth Pushed as commit 94aa2b6. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Update the code review guidelines for JavaFX.
The JavaFX CONTRIBUTING guidelines includes guidance for creating, reviewing, and integrating changes to JavaFX, along with a pointer to a Code Review Policies Wiki page.
This PR updates these guidelines to improve the quality of reviews, with a goal of improving JavaFX and decreasing the chance of introducing a serious regression or other critical bug.
The source branch has three commits:
README-code-reviews.mdwith new guidelinesCONTRIBUTING.mdto highlight important requirements (and minor changes toREADME-code-reviews.md)Commit 1 is content neutral, so it might be helpful for reviewers to look at the changes starting from the second commit.
The updates are:
CONTRIBUTING.mdpage to highlight important requirementsProgress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455$ git checkout pull/1455Update a local copy of the PR:
$ git checkout pull/1455$ git pull https://git.openjdk.org/jfx.git pull/1455/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1455View PR using the GUI difftool:
$ git pr show -t 1455Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1455.diff
Webrev
Link to Webrev Comment