From 9937a00fca638fc62dc6faeb3e2e7a1a6b930a1d Mon Sep 17 00:00:00 2001 From: Katharine Hurst Date: Wed, 6 Nov 2024 15:44:46 +0000 Subject: [PATCH] Updated Readthedocs documentation in line with current practices. --- doc/source/Code-Style-Guide.rst | 13 +++++- doc/source/Guidance-for-Reviewing-Code.rst | 43 ++++++------------- ...ow-to-implement-a-command-line-utility.rst | 10 ++--- doc/source/Running-at-your-site.rst | 23 +++++++--- ...icket-Creation-and-Definition-of-Ready.rst | 14 ++---- 5 files changed, 48 insertions(+), 55 deletions(-) diff --git a/doc/source/Code-Style-Guide.rst b/doc/source/Code-Style-Guide.rst index 28d3046b73..76691dfe6a 100644 --- a/doc/source/Code-Style-Guide.rst +++ b/doc/source/Code-Style-Guide.rst @@ -37,6 +37,9 @@ General comments line interfaces) have acceptance tests. * Use type annotations on function and method interfaces, except for CLIs. +* Avoid putting any functionality within the CLI at all other than + passing in arguments to the plugin, all functionality should be + done within the plugin layer itself. Pull requests ~~~~~~~~~~~~~ @@ -519,7 +522,7 @@ Add a command line interface (improver/cli/.py) to invoke plugins that can be used as a standalone utility or executable within a suite context (e.g. wind downscaling, neighbourhood processing, spot data extraction). These CLIs are invoked using ``bin/improver `` (note that the -CLI filename uses underscores, but the call to use the CLI uses hyphens) +CLI filename uses underscores, but the call to use the CLI uses hyphens). IMPROVER CLIs should only have ``from improver import cli`` as the top level imports. Other imports are placed inside the function that uses @@ -534,6 +537,11 @@ to save a cube to disk, it will need the decorator ``@cli.with_output``, this will mean on the command line, the ``--output`` flag can be used to specify an output path. +As mentioned above, it is important to ensure that no other functionality +than passing inputs into the plugin is done within the CLI layer. +So any checks on the data or requirements of inputs should be done in +the plugin itself. + To load the cubes, each cube argument will need a type. For a basic cube this will be ``cube: cli.inputcube``. If there is a default argument to go with the typed variable, spaces are required around the ``=`` for @@ -765,5 +773,6 @@ New release steps: case just check it. The checksum of the compressed ``.tar.gz`` IMPROVER source code can be obtained via ``openssl sha256 ``. Currently the people with write access to the improver-feedstock - repository are @benfitzpatrick, @PaulAbernethy, @tjtg and @lucyleeow. + repository are @benfitzpatrick, @PaulAbernethy, @tjtg, @cpelley and + @dementipl. You can ping one of these people to merge your pull request. diff --git a/doc/source/Guidance-for-Reviewing-Code.rst b/doc/source/Guidance-for-Reviewing-Code.rst index 8e9198fbf1..ced09fe163 100644 --- a/doc/source/Guidance-for-Reviewing-Code.rst +++ b/doc/source/Guidance-for-Reviewing-Code.rst @@ -8,9 +8,8 @@ As a Developer: branch up to origin. 2. Check the code with respect to the standards in the Definition of Done. Also check that new unit tests do not raise any warnings. -3. Create a pull request. A main reviewer must be assigned, but adding - multiple reviewers to the pull request is encouraged to help share - expertise. Please ask for help assigning reviewers if you need it. +3. Create a pull request and notify a member of the team so it can be + assigned to an appropriate reviewer. 4. The relevant issue should be referenced within the pull request. 5. Respond to the reviewers’ comments and update the pull request accordingly. This will be an iterative process. @@ -26,32 +25,28 @@ As a Developer: perspective, ensure that a subject matter expert is satisfied with the changes. 10. (Internal staff only:) Prior to moving the issue into the 'Done' - column, notify the Product Owner that the issue has been completed, - and discuss the functionality implemented. + column, assign the ticket back to the original developer and notify + them in preparation for merging into the codebase. As a Reviewer: -------------- -1. Issues should be assigned to a main reviewer. Adding multiple - reviewers to the pull request is encouraged to help share expertise. - Please ask for help assigning reviewers if you need it. +1. All reviewers are encouraged to add comments to the pull request. -2. All reviewers are encouraged to add comments to the pull request. - -3. Main reviewers should: +2. Reviewers should: a. **Read the code and post in-line comments for suggested alterations.** b. **Ensure unit tests are run and pass.** c. **Ensure command line interface acceptance tests run and pass. - These must be ran on the desktop using bin/improver tests as they - are not run by travis** + These must be run on the desktop using bin/improver tests, + see :doc:`Running-at-your-site` for more information** d. **The Acceptance Criteria defined within the associated issue has been met.** e. **The criteria within the Definition of Done has been satisfied.** f. **Ensure their testing is documented on the issue.** -4. Main reviewers should post comments to the pull request to show that +4. Reviewers should post comments to the pull request to show that they have completed: a, b, c, d, e, f. 5. Things to consider when reading through the code are: @@ -80,6 +75,7 @@ As a Reviewer: * Does the code do what is supposed to? * Are errors handled appropriately? + * Is the code written to be run efficiently? e. **Test coverage** @@ -92,19 +88,6 @@ As a Reviewer: criteria listed above have been met. The Scrum Master can be consulted, if necessary. -7. If this is a second review, the developer should contact the Product - Owner prior to moving the issue into the 'Done' column. The Scrum - Master can be consulted, if necessary. - -Before merging: ---------------- - -Always rerun Travis before merging a PR. Do this by: - -1. At the bottom of the PR, click ``show all checks`` and then ``details`` next - to the Travis check. -2. Click ``Restart build`` - -If you haven’t already signed into Travis with GitHub it will prompt you -to do this the first time you try this, and it may take a few minutes to -sync before you can see the page with the option to restart the build. +7. If this is a second review, the developer should assign the issue back + to the developer and contact them prior to moving the issue into the + 'Done' column. The Scrum Master can be consulted during the Stand-up. diff --git a/doc/source/How-to-implement-a-command-line-utility.rst b/doc/source/How-to-implement-a-command-line-utility.rst index cbba757144..a13aa7a35c 100644 --- a/doc/source/How-to-implement-a-command-line-utility.rst +++ b/doc/source/How-to-implement-a-command-line-utility.rst @@ -76,13 +76,9 @@ checksums listed in `_. Changes to the checksum file in the code repository identify when changes are made to the code requiring corresponding changes to the data -files. The acceptance test data files are maintained internally in a Met -Office repository, with the directory structure being based on the CLI -plugin names. - -Minimising the usage of input and output data files is desirable in the -future, as described in issue -`#1218 `_. +files. The acceptance test data files are maintained in a public repository: +`https://github.com/metoppv/improver_test_data`, with the directory +structure being based on the CLI plugin names. Use of checksums ~~~~~~~~~~~~~~~~ diff --git a/doc/source/Running-at-your-site.rst b/doc/source/Running-at-your-site.rst index ac7cea27a1..af51de321f 100644 --- a/doc/source/Running-at-your-site.rst +++ b/doc/source/Running-at-your-site.rst @@ -131,24 +131,35 @@ tests are quick to run. Unit tests are run as part of the test suite on # Run unit tests via improver-tests wrapper bin/improver-tests unit + bin/improver-tests --help # Prints out the help information # Use pytest directly with marker to run only the unit tests pytest -m 'not acc' + # To run a particular function within a unit test, you can use the :: notation + pytest -m improver_tests/test_unit_test.py::Test_function The CLI (command line interface) acceptance tests use known good output -(KGO) files on disk for validating that the behaviour is as expected. -These data files are large, so the acceptance tests are not run on -Github actions. Contact a `Met Office IMPROVER -contributor `_ to -arrange for a copy of the acceptance test input and output files. +(KGO) files for validating that the behaviour is as expected. This data +can be found in the `improver_test_data` open source repository on GitHub. The path to the acceptance test data is set using the ``IMPROVER_ACC_TEST_DIR`` environment variable. Acceptance tests will be skipped if this environment variable is not defined. +To run the acceptance tests you can use the following: .. code:: bash - export IMPROVER_ACC_TEST_DIR=/path/to/acceptance/data + export IMPROVER_ACC_TEST_DIR=/path/to/acceptance/data/repo # Use pytest marker to run only the acceptance tests pytest -m acc # Acceptance tests can be run significantly faster in parallel using the pytest-xdist plugin pytest -n 8 + # An example of running just one particular acceptance test + pytest -v -s -m acc -k test_cli_name.py + +To run all tests together at once, the following command can be input + +.. code:: bash + + bin/improver-tests # runs all tests + bin/improver-tests unit # runs unit tests + bin/improver-tests --help # Prints out the help information diff --git a/doc/source/Ticket-Creation-and-Definition-of-Ready.rst b/doc/source/Ticket-Creation-and-Definition-of-Ready.rst index dcbdd23a8f..5404d947a3 100644 --- a/doc/source/Ticket-Creation-and-Definition-of-Ready.rst +++ b/doc/source/Ticket-Creation-and-Definition-of-Ready.rst @@ -9,10 +9,10 @@ User stories When creating new tickets you should follow the template provided: - As a X I want Y so that Z - Related issues: #I, #J + As a W I want X so that Y. It is done when Z. + Optional extra information text goes here Acceptance criteria: @@ -49,15 +49,9 @@ Type * Investigative: A more open-ended investigation, into a technical or scientific development, which may or may not lead to future work to implement the results of the investigation. -* Maintenance: Improvements to the design, structuring and/or behaviour of a - function. Can be viewed as the removal of minor technical debt to improve the - supportability and maintainability. -* Optimisation: An optimisation task mostly focussed on improving the - timeliness of processing. -* Review: A review of the current status of a piece of functionality is - required, ahead of starting more work. * Technical Debt: Previous issues have resulted in the accumulation of technical debt. These issues do the groundwork to remove the technical debt. + This also covers code maintenance and optimisation. Inactive: ~~~~~~~~~ @@ -71,7 +65,7 @@ Inactive: reasons, it is not possible to fix the problem described in this issue, or we are acknowledging that we won’t fix this issue as it’s low priority, and alternative solutions may need to be investigated. -* On Hold: Work started on this issue, however, the priority of this +* Paused: Work started on this issue, however, the priority of this issue decreased, and is now On Hold, potentially until a firmer requirement can be established.