On Wed, 15 May 2024 20:07:06 GMT, Phil Race <p...@openjdk.org> wrote:
>> Update the code review guidelines for JavaFX. >> >> The JavaFX >> [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md) >> guidelines includes guidance for creating, reviewing, and integrating >> changes to JavaFX, along with a pointer to a [Code Review >> Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) 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: >> 1. Converts the Code Review Policies Wiki page to a >> [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md) >> file in the repo and updates hyperlinks to the new location. >> 2. Update `README-code-reviews.md` with new guidelines >> 3. Update `CONTRIBUTING.md` to highlight important requirements (and minor >> changes to `README-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: >> >> * In the Overview section, add a list of items for Reviewers, PR authors, >> and sponsoring Committers to verify prior to integration >> * Create a "Guidelines for reviewing a PR" subsection of the Code review >> policies section >> * Create a "Before you integrate or sponsor a PR" subsection of the Code >> review policies section >> * Update the `CONTRIBUTING.md` page to highlight important requirements > > CONTRIBUTING.md line 233: > >> 231: * 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. >> 232: >> 233: 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). > > "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. 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 line 14: > >> 12: ### Reviewers >> 13: >> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on >> the OpenJDK Census. > > We use ".org" now, not ".java.net" Yes, I missed this. I'll update. > README-code-reviews.md line 40: > >> 38: ### 1. The Reviewer role for the OpenJFX Project >> 39: >> 40: 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. > > (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. I'll fix it. > README-code-reviews.md line 77: > >> 75: >> 76: * All substantive feedback has been addressed, especially any objections >> from one with a Reviewer role. >> 77: * 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. > > 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. I'll add something about this. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602296959 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297934 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602298847 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310947
