From 0071be5a1a1e1e7a2855885db94211814eff358a Mon Sep 17 00:00:00 2001 From: Beq Date: Wed, 17 Jul 2024 22:09:25 +0100 Subject: [PATCH] Tidy up the PR guidance a bit a make it less clunky (hopefully) --- .github/pull_request_template.md | 8 +-- FS_PR_GUIDELINES.md | 91 ++++++++++++++++++-------------- 2 files changed, 53 insertions(+), 46 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 701885cab5..1e32891569 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,8 +1,4 @@ ## Firestorm Pull Request Checklist Thank you for contributing to the Phoenix Firestorm Project. -We will endeavour to review you changes and accept/reject/request changes as soon as possible. This may be a few weeks if we are busy however. -Your code is more likely to be accepted without further modifications being asked for if you can tcik the following boxes. - - [ ] You have read and followed the [Firestorm Pull Request Guidelines](FS_PR_GUIDELINES.md) - - [ ] Code has the correct comment tags. - - [ ] Any related JIRA is mentioned in the commit message. - - [ ] A description of the fix, and bug reproduction steps and tests are described if not already in the associated JIRA. +We will endeavour to review you changes and accept/reject/request changes as soon as possible. +Please read and follow the [Firestorm Pull Request Guidelines](https://github.com/firestormviewer/phoenix-firestorm/blob/master/FS_PR_GUIDELINES.md) to reduce the likelihood that we need to ask for "Bureaucratic" changes to make the code comply with our workflows. diff --git a/FS_PR_GUIDELINES.md b/FS_PR_GUIDELINES.md index 919e0c9532..10d858dffa 100644 --- a/FS_PR_GUIDELINES.md +++ b/FS_PR_GUIDELINES.md @@ -1,52 +1,63 @@ -## Firestorm Pull Request Guidelines +# Firestorm Pull Request Guidelines -Thank you for submitting code to Firestorm, we will review it and merge or provide feedback in due course. -To reduce the number of iterations that we request from you it is useful to ensure your PR meets the following guidelines: +Thank you for submitting code to Firestorm; we will review it and merge or provide feedback in due course. +We have written this guide to help you contribute code that meets our needs. It will hopefully reduce the number of iterations required before we can merge the code. 1. **Descriptive Title**: - - Use a clear and descriptive title for the PR. +   - Use a clear and descriptive title for the PR. -2. **Related Issues**: - - Reference any related issues or pull requests by including the JIRA number and description in the commit message header (e.g., `[FIRE-12345] When I click, my head falls off` or `[FIRE-12345] prevent click detaching head`). +1. **Related Issues**: +   - Reference any related issues or pull requests by including the JIRA number and description in the commit message header (e.g., `[FIRE-12345] When I click, my head falls off` or `[FIRE-12345] prevent click detaching head`). -3. **Description**: - - Provide a detailed description of the changes. Explain why the changes are necessary and what problem they solve. If there is a JIRA associated then there is no need to duplicate that, but a short summary and explanation of the fix woul dbe appreciated. +1. **Description**: +   - Provide a detailed description of the changes. Explain why the changes are necessary and what problem they solve. If a JIRA is associated with the change, there is no need to duplicate that, but we would appreciate a summary and explanation of the fix. -4. **Comment tags (important)**: - - We use comments to preserve the original upstream (LL) code when making modifications. Thsi allows the person merging future code updates to see both the original code from LL, and their updates and use those to determine whether the FS specific changes need to be updated and reviewed. - If you are modifying LL code, we need the LL code preserved in a comment. - For example: - ```c++ - int buggy_code = TRUE; - LL_WARN() << "This code is buggy" << LL_ENDL; - ``` - would become: - ```c++ - // [FIRE-999] Fix the buggy code - // int buggy_code = TRUE; - // LL_WARN() << "This code is buggy" << LL_ENDL; - bool fixed_code = true; - LL_DEBUG() << "I fixed this" << LL_ENDL; - // - ``` - Note: You can tag them with your initials e.g. `` or a short unique tag (shorter is better) +1. **Comment tags (important)**: +   - We use comments to preserve the original upstream (LL) code when making modifications; this allows the person merging future code updates to see both the original code from LL and any new updates and then use those to determine whether the FS-specific changes need to be updated and reviewed. + If you are modifying LL code, we need the LL code preserved in a comment. + For example: - If you are simply adding new code then the same rules apply but there is nothing to comment out. - This is done so that when LL update the original code we can see what the original code was doing, what their changes do and how that relates to the changes that we applied on top. + ```c++ +    int buggy_code = TRUE; +    LL_WARN() << "This code is buggy" << LL_ENDL; + ``` - A single line change, can use the shorthand ``: - ```c++ - bool break_stuff=true; - ``` - could be fixed as follows: - ```c++ - bool break_stuff=false; // [FIRE-23456] don't break stuff. - ``` +Would become: -4. **Testing**: - - Include details on how the changes were tested. Describe the testing environment and any steps needed to verify the changes. + ```c++ +    // [FIRE-999] Fix the buggy code +    // int buggy_code = TRUE; +    // LL_WARN() << "This code is buggy" << LL_ENDL; +    bool fixed_code = true; +    LL_DEBUG() << "I fixed this" << LL_ENDL; +    // + ``` -5. **Documentation**: - - If the change includes a new feature it would be really helpful to suggest how the FS Wiki pages might be updated to help users understand the feature + Note: You can tag them with your initials, e.g. `` or a short unique tag (shorter is better) + + If you add new code, the same rules apply, but there is nothing to comment out. + This is done so that when LL updates the original code, we can see what the original code was doing, what their changes do, and how that relates to the changes that we applied on top. + + A single line change can use the shorthand ``: + + ```c++ +    bool break_stuff=true; + ``` + +Could be fixed as follows: + + ```c++ +    bool break_stuff=false; // [FIRE-23456] don't break stuff. + ``` + + The Comment tags are only required when changing code maintained upstream. If the code you are changing is in an FS-created file, RLV code, OpenSim-only code, etc., then we do not need the comments. + + If the code you are changing is already inside an `//` comment block, then there is no need to add a new block, but do try to make sure that any comments align with the updates you make. + +5. **Testing**: +   - Include details on how the changes should be tested. Describe the testing environment and any steps needed to verify the changes. + +1. **Documentation**: +   - If the change includes a new feature, it would be beneficial to suggest how we should update the FS Wiki pages to help users understand the feature Thank you for your contribution!