merge: #39 from ginger/standardise

.github/PULL_REQUEST_TEMPLATE.md

@@ -0,0 +1,4 @@
+- [ ] I have read the [section on commits and pull requests](https://github.com/NaturalHistoryMuseum/ckanext-iiif/blob/main/CONTRIBUTING.md#commits-and-pull-requests) in `CONTRIBUTING.md`
+
+
+Describe your changes, tagging relevant issues where possible.

.github/workflows/bump.yml

@@ -7,12 +7,12 @@ on:
 
 jobs:
   bump-version:
-    if: "!startsWith(github.event.head_commit.message, 'bump:')"
+    name: Bump version and create changelog
     runs-on: ubuntu-latest
-    name: "Bump version and create changelog"
+    if: "!startsWith(github.event.head_commit.message, 'bump:')"
     steps:
-      - name: Check out
-        uses: actions/checkout@v3
+      - name: Checkout source code
+        uses: actions/checkout@v4
         with:
           token: ${{ secrets.PERSONAL_ACCESS_TOKEN }}
           fetch-depth: 0

.github/workflows/pull-requests.yml

@@ -0,0 +1,27 @@
+name: Validate pull requests
+
+on:
+  pull_request:
+    types: [opened, edited, reopened, synchronize]
+
+jobs:
+  validate-commits:
+    name: Validate commit messages
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout source code
+        uses: actions/checkout@v4
+      - name: Check commit message format
+        uses: webiny/[email protected]
+        with:
+          allowed-commit-types: 'feat,fix,refactor,perf,docs,style,test,build,ci,chore,new,patch,revert,ui'
+  pre-commit:
+    name: Run pre-commit checks
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout source code
+        uses: actions/checkout@v4
+      - name: Setup Python
+        uses: actions/setup-python@v5
+      - name: Run pre-commit
+        uses: pre-commit/[email protected]

.github/workflows/pypi-publish.yml

@@ -10,17 +10,16 @@ permissions:
 
 jobs:
   deploy:
+    name: Deploy package to PyPI
     runs-on: ubuntu-latest
     steps:
-      - name: Check out
-        uses: actions/checkout@v3
+      - name: Checkout source code
+        uses: actions/checkout@v4
         with:
           token: ${{ secrets.PERSONAL_ACCESS_TOKEN }}
           fetch-depth: 0
       - name: Set up Python
-        uses: actions/setup-python@v3
-        with:
-          python-version: '3.x'
+        uses: actions/setup-python@5
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip

.github/workflows/sync.yml

@@ -7,11 +7,11 @@ on:
 
 jobs:
   sync-branches:
+    name: Sync dev and patch branches to latest commit
     runs-on: ubuntu-latest
-    name: "Sync dev and patch branches to latest commit"
     steps:
-      - name: Check out
-        uses: actions/checkout@v3
+      - name: Checkout source code
+        uses: actions/checkout@v4
         with:
           token: ${{ secrets.PERSONAL_ACCESS_TOKEN }}
           fetch-depth: 0

.github/workflows/main.yml → .github/workflows/tests.yml

@@ -3,18 +3,18 @@ name: Tests
 on:
   push:
   workflow_dispatch:
+  pull_request:
+    types: [opened, edited, reopened, synchronize]
 
 jobs:
   test:
+    name: Run tests
     runs-on: ubuntu-latest
-
     steps:
       - name: Checkout source code
-        uses: actions/checkout@v3
-
+        uses: actions/checkout@v4
       - name: Build images
         run: docker compose build
-
       - name: Run tests
         env:
           COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }}

.pre-commit-config.yaml

@@ -1,33 +1,34 @@
 exclude: /(vendor|dist)/
 repos:
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v4.3.0
+    rev: v5.0.0
     hooks:
       - id: check-merge-conflict
       - id: detect-private-key
       - id: end-of-file-fixer
       - id: name-tests-test
-        args: ["--pytest-test-first"]
+        args: ['--pytest-test-first']
         exclude: ^tests/helpers/
       - id: trailing-whitespace
   - repo: https://github.com/commitizen-tools/commitizen
-    rev: v2.37.0
+    rev: v3.30.0
     hooks:
       - id: commitizen
-        additional_dependencies: ["cz-nhm"]
-  - repo: https://github.com/psf/black
-    rev: 22.10.0
+        additional_dependencies: ['cz-nhm']
+  - repo: https://github.com/astral-sh/ruff-pre-commit
+    rev: v0.7.1
     hooks:
-      - id: black
+      - id: ruff
+        args: [ '--fix', '--select', 'I', '--select', 'F401', '--fix-only' ]
+      - id: ruff-format
   - repo: https://github.com/PyCQA/docformatter
-    rev: v1.5.0
+    rev: eb1df34
     hooks:
       - id: docformatter
-        # these can't be pulled directly from the config atm, not sure why
-        args: ["-i", "--wrap-summaries=88", "--wrap-descriptions=88",
-               "--pre-summary-newline", "--make-summary-multi-line"]
+        args: [ '-i', '--config', './pyproject.toml' ]
+        additional_dependencies: ['tomli']
   - repo: https://github.com/pre-commit/mirrors-prettier
-    rev: v3.0.0-alpha.4
+    rev: v4.0.0-alpha.8
     hooks:
       - id: prettier
         types_or: [ javascript, vue, less, sass, scss, css ]

CONTRIBUTING.md

@@ -41,7 +41,7 @@ This extension and [several others](https://github.com/search?q=topic:ckan+org:N
 
 The current core team consists of:
 - Josh ([@jrdh](https://github.com/jrdh)) - Technical Lead
-- Ginger ([@alycejenni](https://github.com/alycejenni)) - Software Engineer
+- Ginger ([@alycejenni](https://github.com/alycejenni)) - Senior Software Engineer
 
 
 ## Questions
@@ -81,7 +81,10 @@ The process is generally as follows:
 3. Make your changes, and commit often; each commit should only contain one change. See below for specifics on how to word your commits.
 4. Push your changes back to your fork.
 5. [Open a pull request](https://docs.github.com/en/get-started/quickstart/contributing-to-projects#making-a-pull-request) in this repository, with the base branch set to **dev** and the compare branch set to your new branch. Provide a summary of your changes in the description.
-6. If the automatic tests fail (these may take a while), please go back to your code and try to make them pass. You may have to update the tests themselves. You don't have to close the pull request while you're doing this; it'll update as you add further commits.
+6. There are several automated checks that will run when you open the pull request. Try to make all of them pass. If you do not at least _attempt_ to make them pass, we will not merge your pull request.
+   1. Tests. Update them so that they pass, if necessary. New tests are always welcome in any pull request, but if you have added a new feature that has decreased the coverage, new tests are required.
+   2. Commit format validation. If you have not followed the conventional commits format for one or more of your commits, this will fail.
+   3. Code format validation. If you have not formatted your code correctly (using Ruff, docformatter, and/or Prettier), this will fail.
 7. Wait for feedback from one of the core maintainers. If it's been a week or so and we haven't responded, we may not have seen it. You can find other places to contact us in [SUPPORT.md](./.github/SUPPORT.md).
 
 ### Commits
@@ -142,9 +145,9 @@ cz c
 
 ##### pre-commit
 
-pre-commit is a tool that runs a variety of checks and modifications before a commit is made. You can check the [.pre-commit-config.yaml](./.pre-commit-config.yaml) file to see eaxtly what it's currently configured to do for this repository, but of particular note:
+pre-commit is a tool that runs a variety of checks and modifications before a commit is made. You can check the [.pre-commit-config.yaml](./.pre-commit-config.yaml) file to see exactly what it's currently configured to do for this repository, but of particular note:
 
-- reformats Python code with [Black](https://github.com/psf/black)
+- reformats Python code with [Ruff](https://docs.astral.sh/ruff)
 - reformats JavaScript and stylesheets with [Prettier](https://prettier.io)
 - reformats docstrings with [docformatter](https://github.com/PyCQA/docformatter)
 - checks your commit message is correcly formatted
@@ -161,15 +164,15 @@ pre-commit run
 
 Don't forget to stage any modifications that it makes! Once it runs without failing, then you can make your commit.
 
-Something to remember is that empty docstrings will cause conflicts between Black and docformatter and the checks will fail repeatedly - so don't leave your docstrings empty!
+Something to remember is that empty docstrings will cause conflicts between Ruff and docformatter and the checks will fail repeatedly - so don't leave your docstrings empty!
 
 ### Code changes and style guide
 
-We generally use external style guides and tools to help us maintain standardised code. Black and Prettier will be run with pre-commit.
+We generally use external style guides and tools to help us maintain standardised code. Ruff and Prettier will be run with pre-commit.
 
 #### Python
 
-We follow the [Black style](https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html), with the notable exception that we use single quotes.
+We use [Ruff](https://docs.astral.sh/ruff) to format our code, using defaults for everything except quote style (we use single quotes).
 
 We also _mostly_ use [CKAN's style](http://docs.ckan.org/en/latest/contributing/python.html), with the following exceptions:
 - prefer `f''` strings over `.format()`
@@ -178,7 +181,7 @@ We also _mostly_ use [CKAN's style](http://docs.ckan.org/en/latest/contributing/
 
 #### JavaScript and stylesheets (CSS, LESS, etc)
 
-We use [Prettier](https://prettier.io) to format these files.
+We use [Prettier](https://prettier.io) to format these files. As with Ruff, we use defaults for everything except quote style (we use single quotes).
 
 #### Accessibility
 

ckanext/iiif/builders/abc.py

@@ -1,6 +1,5 @@
-from typing import Optional
-
 import abc
+from typing import Optional
 
 
 class IIIFResourceBuilder(abc.ABC):
@@ -12,8 +11,7 @@ class IIIFResourceBuilder(abc.ABC):
     """
 
     @abc.abstractmethod
-    def match_and_build(self, identifier: str) -> Optional[dict]:
-        ...
+    def match_and_build(self, identifier: str) -> Optional[dict]: ...
 
     @abc.abstractmethod
     def build_identifier(self, **kwargs) -> str:

ckanext/iiif/builders/manifest.py

@@ -1,13 +1,14 @@
 import re
+from typing import Dict, List, Optional, Union
+
 from ckan import model
 from ckan.common import config
 from ckan.lib.helpers import url_for_static_or_external
 from ckan.logic import NotFound
 from ckan.plugins import toolkit
-from typing import List, Dict, Optional, Union
 
 from .abc import IIIFResourceBuilder
-from .utils import create_id_url, wrap_language, IIIFBuildError
+from .utils import IIIFBuildError, create_id_url, wrap_language
 
 
 class RecordManifestBuilder(IIIFResourceBuilder):
@@ -20,7 +21,7 @@ def build_identifier(self, resource_id: str, record_id: Union[str, int]) -> str:
 
         :param resource_id: the resource ID
         :param record_id: the record ID
-        :return: the identifier
+        :returns: the identifier
         """
         return RecordManifestBuilder._build_record_manifest_id(
             resource_id, str(record_id)
@@ -34,9 +35,9 @@ def match_and_build(self, identifier: str) -> Optional[dict]:
         exceptions.
 
         :param identifier: the manifest ID
-        :return: the manifest as a dict or None if the identifier wasn't a match to the
-                 required format
-        :raise: IIIFBuildError if anything goes wrong after the identifier is matched
+        :returns: the manifest as a dict or None if the identifier wasn't a match to the
+            required format
+        :raises IIIFBuildError: if anything goes wrong after the identifier is matched
         """
         regex = re.compile(
             'resource/(?P<resource_id>.+?)/record/(?P<record_id>[^/]+).*$'
@@ -70,8 +71,8 @@ def build_record_manifest(resource: dict, record: dict) -> Optional[dict]:
 
         :param resource: the resource dict
         :param record: the record data
-        :return: the IIIF manifest for the record and its images
-        :raise: IIIFBuildError if no images are present on the record
+        :returns: the IIIF manifest for the record and its images
+        :raises IIIFBuildError: if no images are present on the record
         """
         manifest_id = RecordManifestBuilder._build_record_manifest_id(resource, record)
 
@@ -115,7 +116,7 @@ def _build_record_manifest_id(
 
         :param resource: the resource dict or resource ID
         :param record: the record dict or record ID
-        :return: the manifest ID
+        :returns: the manifest ID
         """
         resource_id = resource['id'] if isinstance(resource, dict) else resource
         record_id = record['_id'] if isinstance(record, dict) else record
@@ -130,7 +131,7 @@ def _build_label(resource: dict, record: dict) -> Dict[str, List[str]]:
 
         :param resource: the resource dict
         :param record: the record dict
-        :return: the label to use for this manifest
+        :returns: the label to use for this manifest
         """
         title_field = resource.get('_title_field')
         if not title_field:
@@ -146,7 +147,7 @@ def _build_rights(resource: dict) -> str:
         specified on the resource then cc-by is used as a default.
 
         :param resource: the resource dict
-        :return: the license URL to use
+        :returns: the license URL to use
         """
         license_id = resource.get('_image_licence', None)
         # if the license is '' or None we override it
@@ -163,7 +164,7 @@ def _build_metadata(record: dict) -> List[Dict[str, Dict[str, list]]]:
         returned list.
 
         :param record: the record dict
-        :return: a list of language wrapped labels and values
+        :returns: a list of language wrapped labels and values
         """
         # TODO: handle nested dicts and lists
         metadata = []
@@ -186,7 +187,7 @@ def _build_canvas(manifest_id: str, image_number: int, image_id: str) -> dict:
         :param manifest_id: the manifest id
         :param image_number: the image number on the record
         :param image_id: the image URL
-        :return: the canvas definition
+        :returns: the canvas definition
         """
         canvas_id = create_id_url(f'{manifest_id}/canvas/{image_number}')
         annotation_page_id = f'{canvas_id}/0'
@@ -240,7 +241,7 @@ def _get_images(resource: dict, record: dict) -> List[str]:
 
         :param resource: the resource dict
         :param record: the record data dict
-        :return: a list of image URLs
+        :returns: a list of image URLs
         """
         image_field = resource.get('_image_field')
         if not image_field or image_field not in record:

ckanext/iiif/builders/utils.py

@@ -1,13 +1,14 @@
-from ckan.plugins import toolkit
 from typing import Dict, List, Union
 
+from ckan.plugins import toolkit
+
 
 def create_id_url(identifier: str) -> str:
     """
     Given the identifier of a IIIF resource, creates the full URL for it.
 
     :param identifier: the IIIF resource ID
-    :return: the full URL for the IIIF resource (e.g. a manifest)
+    :returns: the full URL for the IIIF resource (e.g. a manifest)
     """
     return toolkit.url_for('iiif.resource', identifier=identifier, _external=True)
 
@@ -21,7 +22,7 @@ def wrap_language(
 
     :param value: the value/values
     :param language: the language, defaults to 'none'
-    :return: the value in the right IIIF language format
+    :returns: the value in the right IIIF language format
     """
     if not isinstance(value, list):
         value = [value]

ckanext/iiif/interfaces.py

@@ -1,5 +1,6 @@
+from typing import Callable, Optional, OrderedDict
+
 from ckan.plugins import interfaces
-from typing import Optional, Callable, OrderedDict
 
 
 class IIIIF(interfaces.Interface):

ckanext/iiif/logic/actions.py

@@ -1,9 +1,10 @@
+import logging
 from collections import OrderedDict
+from typing import Optional
+from typing import OrderedDict as OrderedDictType
 
-import logging
 from ckan.plugins import toolkit
 from ckantools.decorators import action
-from typing import Optional, OrderedDict as OrderedDictType
 
 from ..builders.abc import IIIFResourceBuilder
 from ..builders.manifest import RecordManifestBuilder
@@ -38,7 +39,7 @@ def build_iiif_resource(identifier: str) -> Optional[dict]:
     None is returned.
 
     :param identifier: the IIIF resource identifier
-    :return: a dict or None
+    :returns: a dict or None
     """
     for builder in BUILDERS.values():
         try: