From 704119c669c71832b70519cc718e046d86c232c4 Mon Sep 17 00:00:00 2001 From: jennyhickson Date: Fri, 17 Oct 2025 16:26:07 +0100 Subject: [PATCH 1/3] guidance on issues --- source/Development/documentation.rst | 5 ++-- source/WorkingPractices/gh_dev_init.rst | 33 ++++++++++++++++++++----- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/source/Development/documentation.rst b/source/Development/documentation.rst index 3055116a..d2a4559f 100644 --- a/source/Development/documentation.rst +++ b/source/Development/documentation.rst @@ -35,8 +35,9 @@ alongside the code. This is then compiled using Sphinx into the webpages above. The UM documentation papers are written in LaTeX and stored in a separate repository. Guidelines for editing the UM documentation papers are available - here `https://code.metoffice.gov.uk/trac/um/wiki/WorkingPractices/Documentation - /UpdatingUMDPs`_. + `here `_ + +.._ umdp: https://code.metoffice.gov.uk/trac/um/wiki/WorkingPractices/Documentation/UpdatingUMDPs LFRic Apps and Core also use doxygen to document the code and all changes should include appropriate doxygen changes to go with them. Doxygen guidelines diff --git a/source/WorkingPractices/gh_dev_init.rst b/source/WorkingPractices/gh_dev_init.rst index 7e1d2c70..6265b514 100644 --- a/source/WorkingPractices/gh_dev_init.rst +++ b/source/WorkingPractices/gh_dev_init.rst @@ -10,6 +10,33 @@ created a fork (:ref:`forking`). Create an Issue --------------- +An issue in github can be used to document a new feature, bug, or other problem +in the codebase. There are issue templates provided to help capture all the +relevant information in each case. + +They are also the main place to document the development process of a change. +Sub-issues can be created if a large piece of work wants breaking down into +smaller sections. If you are working on an issue, then assign yourself to it so +that others know that you are working on it. Issues are created in the upstream +repository. + +.. admonition:: Before Opening an Issue + + **Are you using the latest version?** If not, please update to the latest + stable release and verify the issue persists. + + **Is this a security issue?** Please do not file a public issue for + security vulnerabilities, but get in touch with the :ref:`SSD team ` + directly. + + **Is this a duplicate?** Add a comment or an emoji reaction to the original + issue rather than opening a new one. + + **Is this a support request?** Questions should be asked via the + `simulation-systems discussion boards`_ where we'll be happy to try and help. + +.. _simulation-systems discussion boards: https://github.com/MetOffice/simulation-systems/discussions + .. important:: It is not guaranteed that opening an issue will result in action or even @@ -17,12 +44,6 @@ Create an Issue or individual should be aware of an issue, then contact them directly in addition to opening an issue. -An issue in github can be used to document a problem in the codebase or as -somewhere to document the development process for a new feature. Sub-issues -can also be created if a large piece of work wants breaking down into smaller -sections. If you are working on an issue, then assign yourself to it so that -others know that you are working on it. Issues are created in the upstream -repository. .. tab-set:: From 8b0c07f05b23819640e6c9080ff3fae530a471eb Mon Sep 17 00:00:00 2001 From: jennyhickson Date: Fri, 17 Oct 2025 17:00:34 +0100 Subject: [PATCH 2/3] guidance on reviews --- source/Development/planning_your_change.rst | 22 ++++++++++++++------- source/Reviewers/codereview.rst | 4 ++++ source/Reviewers/scitechreview.rst | 5 +++++ source/WorkingPractices/gh_dev_init.rst | 4 +++- source/WorkingPractices/pull_requests.rst | 3 +++ 5 files changed, 30 insertions(+), 8 deletions(-) diff --git a/source/Development/planning_your_change.rst b/source/Development/planning_your_change.rst index 0626d244..23ee6d53 100644 --- a/source/Development/planning_your_change.rst +++ b/source/Development/planning_your_change.rst @@ -33,13 +33,18 @@ The following are some general hints and tips in planning code changes successfu General Considerations ---------------------- +**Smaller is better.** Submit *one* pull request per bug fix or feature. Do not +refactor or reformat code that is unrelated to your change. It is better to +submit many small pull requests rather than a single large one. Smaller, well- +scoped pull requests are easier to review and more likely to be accepted promptly. -**How complex is your change likely to be?** (e.g. roughly how many subroutines -or lines of code do you expect to alter or add?) This is an important -consideration as the more complex a change is, the more time will be required -in development, the more code owners will need to approve it and so forth. If a -change is overly complex, the developer should consider breaking it up into -smaller, more manageable and, where possible, "self contained" tickets. +**How complex is your change likely to be?** For larger, more complicated changes +start by opening an issue and discussing a strategy with the relevant people. This +helps avoid unnecessary work and ensures alignment with project goals. + +**Prioritise clarity over cleverness.** Code is read far more often than it is +written so aim for code that is easy to understand and maintain. If the logic +is not immediately obvious then include comments to explain your reasoning. **How does your proposed change fit in with the structure of the model?** Try and make your code changes in-scope and no larger than they need to be. If you @@ -54,7 +59,7 @@ aware of these. * `UMDP3 (UM and JULES FORTRAN) `__ * `LFRic Coding Styles - `__ + `__ * `PEP 8 (Python) `__ **Who will SciTech review the change?** This is a useful consideration as not @@ -78,6 +83,9 @@ linked tickets. See :ref:`multirepo` for further details. idea **not** to re-invent the wheel or have code duplication! Speaking to code owners of the appropriate sections can help in this instance. +**How will your change be tested?** Include unit or integration tests, and update +any example or demo repositories to exercise new functionality. + Specific Tips for Scientific changes ------------------------------------ diff --git a/source/Reviewers/codereview.rst b/source/Reviewers/codereview.rst index 9fdcc6d6..5f1114d9 100644 --- a/source/Reviewers/codereview.rst +++ b/source/Reviewers/codereview.rst @@ -20,6 +20,10 @@ Fundamentally this review is to ensure that the change is well thought-out and that no system aspects have been missed. The review should be an active one and should question anything that is poorly coded. +Focus on the code, not the contributor; providing constructive, respectful and +actionable feedback. Critique the implementation, not the individual and always +explain the reasoning behind your suggestions. + Reviewer responsibilities and checkpoints ----------------------------------------- diff --git a/source/Reviewers/scitechreview.rst b/source/Reviewers/scitechreview.rst index 44d2af5b..4e440227 100644 --- a/source/Reviewers/scitechreview.rst +++ b/source/Reviewers/scitechreview.rst @@ -16,6 +16,11 @@ Purpose of the review The purpose of code review is to ensure that the code does the job it says it performs, is standards compliant and well documented. +Focus on the code, not the contributor; providing constructive, respectful and +actionable feedback. Critique the implementation, not the individual and always +explain the reasoning behind your suggestions. + + Reviewer responsibilities and checkpoints ----------------------------------------- diff --git a/source/WorkingPractices/gh_dev_init.rst b/source/WorkingPractices/gh_dev_init.rst index 6265b514..4329a0b0 100644 --- a/source/WorkingPractices/gh_dev_init.rst +++ b/source/WorkingPractices/gh_dev_init.rst @@ -216,7 +216,9 @@ And then commit the change, information see the relevant man page, ``man git add``. Finally, you may want to push any commits stored in your local clone back to -the remote source. +the remote source. This allows you to share code with someone else during the +development process, or prepare to open a pull request once you're ready for a +review. .. code-block:: diff --git a/source/WorkingPractices/pull_requests.rst b/source/WorkingPractices/pull_requests.rst index c461ae05..2ac64994 100644 --- a/source/WorkingPractices/pull_requests.rst +++ b/source/WorkingPractices/pull_requests.rst @@ -40,6 +40,9 @@ Trivial pull requests are an exception and do not require a SciTech review. close that part of the review. The developer should **not** resolve conversations themselves. + * Feedback on code is not a refection of personal ability. All code can be + improved and reviews are an opportunity for shared learning and collaboration. + Opening a Pull Request ---------------------- From 8926d6f5b4a5c6725dc95e1969987fb6b2c4f855 Mon Sep 17 00:00:00 2001 From: jennyhickson Date: Mon, 20 Oct 2025 08:35:41 +0100 Subject: [PATCH 3/3] review suggestions --- source/Development/planning_your_change.rst | 24 ++++++++++----------- source/WorkingPractices/pull_requests.rst | 6 ++---- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/source/Development/planning_your_change.rst b/source/Development/planning_your_change.rst index 23ee6d53..b0f42447 100644 --- a/source/Development/planning_your_change.rst +++ b/source/Development/planning_your_change.rst @@ -33,18 +33,18 @@ The following are some general hints and tips in planning code changes successfu General Considerations ---------------------- -**Smaller is better.** Submit *one* pull request per bug fix or feature. Do not -refactor or reformat code that is unrelated to your change. It is better to -submit many small pull requests rather than a single large one. Smaller, well- -scoped pull requests are easier to review and more likely to be accepted promptly. - -**How complex is your change likely to be?** For larger, more complicated changes -start by opening an issue and discussing a strategy with the relevant people. This -helps avoid unnecessary work and ensures alignment with project goals. - -**Prioritise clarity over cleverness.** Code is read far more often than it is -written so aim for code that is easy to understand and maintain. If the logic -is not immediately obvious then include comments to explain your reasoning. +**Smaller is better.** Submit a separate pull request per bug fix or feature. +Avoid refactoring or reformating code that is not related to your change. +Multiple small pull requests are easier to review and more likely to be accepted +promptly. + +**Consider the complexity of your change.** For larger or more complex changes, +start by opening an issue and discussing your approach with the relevant people. +This helps avoid unnecessary work and ensures alignment with project goals. + +**Prioritise clarity over cleverness.** Code is read more often than it is +written, so make it easy to understand and maintain. If the logic is not +immediately obvious then include comments to explain your reasoning. **How does your proposed change fit in with the structure of the model?** Try and make your code changes in-scope and no larger than they need to be. If you diff --git a/source/WorkingPractices/pull_requests.rst b/source/WorkingPractices/pull_requests.rst index 2ac64994..f44f3de7 100644 --- a/source/WorkingPractices/pull_requests.rst +++ b/source/WorkingPractices/pull_requests.rst @@ -17,12 +17,10 @@ Trivial pull requests are an exception and do not require a SciTech review. :ref:`Linked pull requests ` will move through the review states together. -.. tip:: - - Some things to consider when getting ready for review: +.. admonition:: Getting Ready for Review * If your development changes answers then make sure you have followed the - steps on :ref:`preparing a KGO ticket for review.` + steps on :ref:`preparing a KGO pull request for review.` * Get in touch with your SciTech Reviewer before you feel ready for review. They will have valuable insights into the code and, particularly for larger