Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-22 Thread Johan Vos
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-22 Thread Kevin Rushforth
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-22 Thread John Hendrikx
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-21 Thread Nir Lisker
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-21 Thread Kevin Rushforth
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-21 Thread Kevin Rushforth
On Sat, 18 May 2024 14:28:31 GMT, Nir Lisker wrote: >> 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-21 Thread Andy Goryachev
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v4]

2024-05-21 Thread Kevin Rushforth
> 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-18 Thread Nir Lisker
On Sat, 18 May 2024 14:24:49 GMT, Kevin Rushforth wrote: > Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"? This looks like a good idea. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605798488

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-18 Thread Kevin Rushforth
On Fri, 17 May 2024 15:02:14 GMT, Nir Lisker wrote: >> Kevin Rushforth has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 20 additional >> commits

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Nir Lisker
On Fri, 17 May 2024 14:58:41 GMT, Nir Lisker wrote: >> Kevin Rushforth has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 20 additional >> commits

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Nir Lisker
On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v3]

2024-05-17 Thread Andy Goryachev
On Fri, 17 May 2024 14:55:18 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v3]

2024-05-17 Thread Kevin Rushforth
> 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:18:24 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 79: >> >>> 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Thu, 16 May 2024 07:27:34 GMT, Johan Vos wrote: >> That's a good idea. > > 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 18:30:06 GMT, Andy Goryachev wrote: >> Kevin Rushforth has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 20 additional >>

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 19:38:55 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 64: >> >>> 62: >>> 63: * Make sure you understand why there was an issue to begin with, and >>> why/how the proposed PR solves the issue >>> 64: * Focus first on substantive comments rather than stylistic

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:00:42 GMT, Kevin Rushforth wrote: >> 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 > > It's `/reviewers 2 reviewers`, so I'll add that as

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 21:58:46 GMT, Kevin Rushforth wrote: >> 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.

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:13:34 GMT, Kevin Rushforth wrote: >>> or is it? :-) >> >> ![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293) >> >> Looks to me like it is... > > A passing GHA test run is neither necessary nor sufficient. It is an > interesting

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:10:12 GMT, Kevin Rushforth wrote: > > ...inadvertent introduction of new API (that will have to be deprecated if > > missed) > > I think this is worth mentioning. Fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605063298

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:05:46 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 60: >> >>> 58: * 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 21:57:55 GMT, Kevin Rushforth wrote: >> 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 >>

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
> 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

Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-17 Thread Kevin Rushforth
On Thu, 16 May 2024 08:48:14 GMT, John Hendrikx wrote: >> 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-16 Thread John Hendrikx
On Thu, 16 May 2024 07:30:31 GMT, Johan Vos wrote: >> 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. > > I agree with the concern, but I still think it's much better to

Re: RFR: 8332313: Update code review guidelines

2024-05-16 Thread Johan Vos
On Wed, 15 May 2024 21:57:23 GMT, Kevin Rushforth wrote: >> 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

Re: RFR: 8332313: Update code review guidelines

2024-05-16 Thread Johan Vos
On Wed, 15 May 2024 19:39:10 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 66: >> >>> 64: * Focus first on substantive comments rather than stylistic comments >>> 65: * Consider the risk of regression >>> 66: * Consider any compatibility concerns >> >> regression and compatibility

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:18:05 GMT, Phil Race wrote: >> This is out of scope for this PR, but it might be something to consider >> addressing in a follow-on enhancement. > > It should be made clear (somewhere, at some point) that it would be rare for > a CSR to be completed and approved before

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:08:05 GMT, John Hendrikx 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 19:50:32 GMT, Andy Goryachev wrote: >> README-code-reviews.md line 60: >> >>> 58: * 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:23:37 GMT, Nir Lisker wrote: >> Isn't this automatic? Seems weird you could integrate without these passing. > >> or is it? :-) > > ![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293) > > Looks to me like it is... A passing GHA

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:12:09 GMT, Phil Race wrote: >> README-code-reviews.md line 48: >> >>> 46: 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 19:04:19 GMT, Nir Lisker 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:33:37 GMT, Andy Goryachev wrote: >> 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 20:07:06 GMT, Phil Race 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 20:03:01 GMT, Phil Race wrote: >> Not sure what you mean, but it doesn't seem related to this PR. > > 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Nir Lisker
On Wed, 15 May 2024 20:08:10 GMT, John Hendrikx wrote: >> or is it? :-) > > Isn't this automatic? Seems weird you could integrate without these passing. > or is it? :-) ![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293) Looks to me like it is...

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 19:00:57 GMT, Nir Lisker 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 19:32:04 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 103: >> >>> 101: 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread John Hendrikx
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread John Hendrikx
On Wed, 15 May 2024 19:53:43 GMT, Andy Goryachev wrote: >> README-code-reviews.md line 68: >> >>> 66: * Consider any compatibility concerns >>> 67: * Check whether there is an automated test; if not, ask for one, if it >>> is feasible >>> 68: * Make sure that the PR has executed the GHA tests

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Phil Race
On Wed, 15 May 2024 18:56:29 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 10: >> >>> 8: >>> 9: __Project Co-Lead__: Kevin Rushforth (kcr) >>> 10: __Project Co-Lead__: Johan Vos (jvos) >> >> There are two sets of ids - one for OpenJFX/JBS and one for Github. This >> might be

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 19:23:25 GMT, Nir Lisker 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 19:16:31 GMT, Nir Lisker 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 >>

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 18:20:22 GMT, Andy Goryachev 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Nir Lisker
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 18:05:10 GMT, Andy Goryachev 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

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Andy Goryachev
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

Re: RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 >

RFR: 8332313: Update code review guidelines

2024-05-15 Thread Kevin Rushforth
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