RFR: Things to consider before PR [v6]
Jesper Wilhelmsson
jwilhelm at openjdk.org
Wed May 3 11:27:18 UTC 2023
On Fri, 28 Apr 2023 14:52:42 GMT, Sean Mullan <mullan at openjdk.org> wrote:
>> Jesper Wilhelmsson has updated the pull request incrementally with one additional commit since the last revision:
>>
>> Few more updates
>
> src/guide/contributing-to-an-open-jdk-project.md line 13:
>
>> 11: ## Things to consider before proposing changes to OpenJDK code
>> 12:
>> 13: Every change in JDK code carries risk of changes in behavior which adversely affect applications. Generally we're looking to improve the functionality and capability and sometimes performance of the platform without that negative consequence. We do have many thousands of tests, but they're inevitably incomplete in coverage. So we need to ask ourselves whether each change is worthwhile - and some may not be no matter how well intentioned.
>
> This sounds like all changes in behavior are negative, which is not true. I suggest inserting "may" before "adversely".
>
> Some other wording suggestions in this paragraph:
>
> "Every change to JDK code ..."
> "carries a risk .."
Fixed.
> src/guide/contributing-to-an-open-jdk-project.md line 13:
>
>> 11: ## Things to consider before proposing changes to OpenJDK code
>> 12:
>> 13: Every change in JDK code carries risk of changes in behavior which adversely affect applications. Generally we're looking to improve the functionality and capability and sometimes performance of the platform without that negative consequence. We do have many thousands of tests, but they're inevitably incomplete in coverage. So we need to ask ourselves whether each change is worthwhile - and some may not be no matter how well intentioned.
>
>> We do have many thousands of tests, but they're inevitably incomplete in coverage.
>
> This sentence needs more explanation, I think. If you are changing code, ideally you would provide tests that test that behavior change if the current tests are inadequate. Does this sentence add anything to the main points in this paragraph - otherwise, maybe it should be removed.
Agreed. Removed.
-------------
PR Review Comment: https://git.openjdk.org/guide/pull/101#discussion_r1183556680
PR Review Comment: https://git.openjdk.org/guide/pull/101#discussion_r1183556772
More information about the guide-dev
mailing list