Merge pull request #74 from lsst/tickets/DM-42116

DM-42116: Fix numpydoc doc strings

.github/workflows/docstyle.yaml

@@ -11,3 +11,18 @@ jobs:
     uses: lsst/rubin_workflows/.github/workflows/docstyle.yaml@main
     with:
       args: "python/lsst/resources/"
+  numpydoc:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Set up Python
+        uses: actions/setup-python@v4
+
+      - name: Install numpydoc
+        run: |
+          python -m pip install --upgrade pip
+          python -m pip install numpydoc
+
+      - name: Validate docstrings
+        run: python -m numpydoc.hooks.validate_docstrings $(find python -name "*.py")

.pre-commit-config.yaml

@@ -16,12 +16,16 @@ repos:
         # https://pre-commit.com/#top_level-default_language_version
         language_version: python3.11
   - repo: https://github.com/pycqa/isort
-    rev: 5.12.0
+    rev: 5.13.1
     hooks:
       - id: isort
         name: isort (python)
   - repo: https://github.com/astral-sh/ruff-pre-commit
     # Ruff version.
-    rev: v0.1.6
+    rev: v0.1.7
     hooks:
       - id: ruff
+  - repo: https://github.com/numpy/numpydoc
+    rev: "v1.6.0"
+    hooks:
+      - id: numpydoc-validation

pyproject.toml

@@ -178,3 +178,21 @@ max-doc-length = 79
 
 [tool.ruff.pydocstyle]
 convention = "numpy"
+
+[tool.numpydoc_validation]
+checks = [
+    "all",  # All except the rules listed below.
+    "SA01",  # See Also section.
+    "EX01",  # Example section.
+    "SS06",  # Summary can go into second line.
+    "GL01",  # Summary text can start on same line as """
+    "GL08",  # Do not require docstring.
+    "ES01",  # No extended summary required.
+    "RT01",  # Unfortunately our @property trigger this.
+    "RT02",  # Does not want named return value. DM style says we do.
+    "SS05",  # pydocstyle is better at finding infinitive verb.
+]
+exclude = [
+    '^__init__$',
+    '\._[a-zA-Z_]+$',  # Private methods.
+]

python/lsst/resources/_resourceHandles/_baseResourceHandle.py

@@ -132,9 +132,9 @@ class BaseResourceHandle(ABC, ResourceHandleProtocol[U]):
     Parameters
     ----------
     mode : `str`
-        Handle modes as described in the python `io` module
+        Handle modes as described in the python `io` module.
     log : `~logging.Logger`
-        Logger to used when writing messages
+        Logger to used when writing messages.
     newline : `str`
         When doing multiline operations, break the stream on given character
         Defaults to newline.

python/lsst/resources/_resourceHandles/_fileResourceHandle.py

@@ -23,14 +23,14 @@
 
 
 class FileResourceHandle(BaseResourceHandle[U]):
-    """File based specialization of `.BaseResourceHandle`
+    """File based specialization of `.BaseResourceHandle`.
 
     Parameters
     ----------
     mode : `str`
-        Handle modes as described in the python `io` module
+        Handle modes as described in the python `io` module.
     log : `~logging.Logger`
-        Logger to used when writing messages
+        Logger to used when writing messages.
     filename : `str`
         Name of the file on the filesystem to use.
     encoding : `str` or None

python/lsst/resources/_resourceHandles/_httpResourceHandle.py

@@ -25,7 +25,27 @@
 
 
 class HttpReadResourceHandle(BaseResourceHandle[bytes]):
-    """HTTP-based specialization of `.BaseResourceHandle`."""
+    """HTTP-based specialization of `.BaseResourceHandle`.
+
+    Parameters
+    ----------
+    mode : `str`
+        Handle modes as described in the python `io` module.
+    log : `~logging.Logger`
+        Logger to used when writing messages.
+    session : `requests.Session`
+        The session to use for this handle.
+    url : `str`
+        URL of remote resource.
+    timeout : `tuple` [`int`, `int`]
+        Timeout to use for connections: connection timeout and read timeout
+        in a tuple.
+    newline : `str` or `None`, optional
+        When doing multiline operations, break the stream on given character.
+        Defaults to newline. If a file is opened in binary mode, this argument
+        is not used, as binary files will only split lines on the binary
+        newline representation.
+    """
 
     def __init__(
         self,

python/lsst/resources/_resourceHandles/_s3ResourceHandle.py

@@ -31,7 +31,7 @@
 
 
 class S3ResourceHandle(BaseResourceHandle[bytes]):
-    """S3 specialization of `.BaseResourceHandle`
+    """S3 specialization of `.BaseResourceHandle`.
 
     Parameters
     ----------
@@ -50,8 +50,8 @@ class S3ResourceHandle(BaseResourceHandle[bytes]):
         When doing multiline operations, break the stream on given character.
         Defaults to newline.
 
-    Note
-    ----
+    Notes
+    -----
     It is only possible to incrementally flush this object if each chunk that
     is flushed is above 5MB in size. The flush command is ignored until the
     internal buffer reaches this size, or until close is called, whichever

python/lsst/resources/_resourcePath.py

@@ -52,7 +52,7 @@
 MAX_WORKERS = 10
 
 
-class ResourcePath:
+class ResourcePath:  # numpydoc ignore=PR02
     """Convenience wrapper around URI parsers.
 
     Provides access to URI components and can convert file
@@ -63,7 +63,7 @@ class ResourcePath:
 
     Parameters
     ----------
-    uri : `str`, `pathlib.Path`, `urllib.parse.ParseResult`, or `ResourcePath`.
+    uri : `str`, `pathlib.Path`, `urllib.parse.ParseResult`, or `ResourcePath`
         URI in string form.  Can be scheme-less if referring to a relative
         path or an absolute path on the local file system.
     root : `str` or `ResourcePath`, optional
@@ -77,7 +77,7 @@ class ResourcePath:
         scheme-less and will not be updated to ``file`` or absolute path unless
         it is already an absolute path, in which case it will be updated to
         a ``file`` scheme.
-    forceDirectory: `bool`, optional
+    forceDirectory : `bool`, optional
         If `True` forces the URI to end with a separator, otherwise given URI
         is interpreted as is.
     isTemporary : `bool`, optional
@@ -404,7 +404,7 @@ def root_uri(self) -> ResourcePath:
         Returns
         -------
         uri : `ResourcePath`
-            root URI.
+            Root URI.
         """
         return self.replace(path="", forceDirectory=True)
 
@@ -534,6 +534,7 @@ def updatedFile(self, newfile: str) -> ResourcePath:
         Returns
         -------
         updated : `ResourcePath`
+            Updated `ResourcePath` with new updated final component.
 
         Notes
         -----

python/lsst/resources/file.py

@@ -87,12 +87,10 @@ def _as_local(self) -> tuple[str, bool]:
         return self.ospath, self.isTemporary
 
     def read(self, size: int = -1) -> bytes:
-        """Return the entire content of the file as bytes."""
         with open(self.ospath, "rb") as fh:
             return fh.read(size)
 
     def write(self, data: bytes, overwrite: bool = True) -> None:
-        """Write the supplied data to the file."""
         dir = os.path.dirname(self.ospath)
         if not os.path.exists(dir):
             _create_directories(dir)
@@ -488,7 +486,7 @@ def _create_directories(name: str | bytes) -> None:
 
     Parameters
     ----------
-    name: `str` or `bytes`
+    name : `str` or `bytes`
         Path to the directory to be created
 
     Notes

python/lsst/resources/gs.py

@@ -89,7 +89,19 @@ class ServiceUnavailable(ClientError):  # type: ignore  # noqa: N818
 
 
 def is_retryable(exc: Exception) -> bool:
-    """Report if the given exception is a condition that can be retried."""
+    """Report if the given exception is a condition that can be retried.
+
+    Parameters
+    ----------
+    exc : `Exception`
+        Exception to check.
+
+    Returns
+    -------
+    `bool`
+        Returns `True` if the given exception is a condition that can be
+        retried.
+    """
     return isinstance(exc, _RETRIEVABLE_TYPES)
 
 

python/lsst/resources/http.py

@@ -60,7 +60,7 @@ def _timeout_from_environment(env_var: str, default_value: float) -> float:
     ----------
     env_var : `str`
         Environment variable to look for.
-    default_value: `float``
+    default_value : `float``
         Value to return if `env_var` is not defined in the environment.
 
     Returns
@@ -426,7 +426,23 @@ def __call__(self, req: requests.PreparedRequest) -> requests.PreparedRequest:
 
 
 class SessionStore:
-    """Cache a reusable HTTP client session per endpoint."""
+    """Cache a reusable HTTP client session per endpoint.
+
+    Parameters
+    ----------
+    num_pools : `int`, optional
+        Number of connection pools to keep: there is one pool per remote
+        host.
+    max_persistent_connections : `int`, optional
+        Maximum number of connections per remote host to persist in each
+        connection pool.
+    backoff_min : `float`, optional
+        Minimum value of the interval to compute the exponential
+        backoff factor when retrying requests (seconds).
+    backoff_max : `float`, optional
+        Maximum value of the interval to compute the exponential
+        backoff factor when retrying requests (seconds).
+    """
 
     def __init__(
         self,
@@ -439,17 +455,15 @@ def __init__(
         # of the dictionary is a root URI and the value is the session.
         self._sessions: dict[str, requests.Session] = {}
 
-        # Number of connection pools to keep: there is one pool per remote
-        # host. See documentation of urllib3 PoolManager class:
+        # See documentation of urllib3 PoolManager class:
         # https://urllib3.readthedocs.io
         self._num_pools: int = num_pools
 
-        # Maximum number of connections per remote host to persist in each
-        # connection pool. See urllib3 Advanced Usage documentation:
+        # See urllib3 Advanced Usage documentation:
         # https://urllib3.readthedocs.io/en/stable/advanced-usage.html
         self._max_persistent_connections: int = max_persistent_connections
 
-        # Minimum and maximum values of the inverval to compute the exponential
+        # Minimum and maximum values of the interval to compute the exponential
         # backoff factor when retrying requests (seconds).
         self._backoff_min: float = backoff_min
         self._backoff_max: float = backoff_max if backoff_max > backoff_min else backoff_min + 1.0
@@ -946,6 +960,8 @@ def transfer_from(
         transfer : `str`
             Mode to use for transferring the resource. Supports the following
             options: copy.
+        overwrite : `bool`, optional
+            Whether overwriting the remote resource is allowed or not.
         transaction : `~lsst.resources.utils.TransactionProtocol`, optional
             Currently unused.
         """
@@ -1119,7 +1135,7 @@ def _send_webdav_request(
         headers : `dict`, optional
             A dictionary of key-value pairs (both strings) to include as
             headers in the request.
-        body: `str`, optional
+        body : `str`, optional
             The body of the request.
 
         Notes
@@ -1322,7 +1338,6 @@ def _copy_or_move(self, method: str, src: HttpResourcePath) -> None:
         method : `str`
             The method to perform. Valid values are "COPY" or "MOVE" (in
             uppercase).
-
         src : `HttpResourcePath`
             The source of the contents to move to `self`.
         """
@@ -1607,6 +1622,11 @@ def _parse_propfind_response_body(body: str) -> list[DavProperty]:
 class DavProperty:
     """Helper class to encapsulate select live DAV properties of a single
     resource, as retrieved via a PROPFIND request.
+
+    Parameters
+    ----------
+    response : `eTree.Element` or `None`
+        The XML response defining the DAV property.
     """
 
     # Regular expression to compare against the 'status' element of a

python/lsst/resources/packageresource.py

@@ -99,7 +99,6 @@ def exists(self) -> bool:
         return ref.is_file() or ref.is_dir()
 
     def read(self, size: int = -1) -> bytes:
-        """Read the contents of the resource."""
         ref = self._get_ref()
         if not ref:
             raise FileNotFoundError(f"Unable to locate resource {self}.")

python/lsst/resources/s3.py

@@ -51,7 +51,19 @@
 
 
 class ProgressPercentage:
-    """Progress bar for S3 file uploads."""
+    """Progress bar for S3 file uploads.
+
+    Parameters
+    ----------
+    file : `ResourcePath`
+        Resource that is relevant to the progress percentage. The size of this
+        resource will be used to determine progress. The name will be used
+        in the log messages unless overridden by ``file_for_msg``.
+    file_for_msg : `ResourcePath` or `None`, optional
+        Resource name to include in log messages in preference to ``file``.
+    msg : `str`, optional
+        Message text to be included in every progress log message.
+    """
 
     log_level = logging.DEBUG
     """Default log level to use when issuing a message."""
@@ -152,7 +164,6 @@ def remove(self) -> None:
 
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
     def read(self, size: int = -1) -> bytes:
-        """Read the contents of the resource."""
         args = {}
         if size > 0:
             args["Range"] = f"bytes=0-{size-1}"
@@ -170,7 +181,6 @@ def read(self, size: int = -1) -> bytes:
 
     @backoff.on_exception(backoff.expo, all_retryable_errors, max_time=max_retry_time)
     def write(self, data: bytes, overwrite: bool = True) -> None:
-        """Write the supplied data to the resource."""
         if not overwrite and self.exists():
             raise FileExistsError(f"Remote resource {self} exists and overwrite has been disabled")
         with time_this(log, msg="Write to %s", args=(self,)):