From 67bb4ecb25c7841bde9a7ba0b7497f618112513a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=B6rnquist?= <hugo@lysator.liu.se> Date: Sat, 1 Oct 2022 01:41:50 +0200 Subject: [PATCH] Harden documentation constraints. Remove ignore of D100 (Missing docstring in public module) from pydocstyle, and subsequentially introduce module documentation of all module. At the same time, improve general documentation of code, while moving some functions to be private to simplify the code. --- r11k/__main__.py | 2 + r11k/cache.py | 19 +++++++--- r11k/config.py | 9 +++++ r11k/forge.py | 60 +++++++++++++++++++----------- r11k/gitutil.py | 8 +++- r11k/httpcache.py | 2 + r11k/interval.py | 61 +++++++++++++++++++++++++----- r11k/main.py | 11 +++++- r11k/puppet.py | 70 +++++++++++++++++++++++++++-------- r11k/puppetmodule/__init__.py | 20 ++++++++-- r11k/puppetmodule/base.py | 46 ++++++++++++++++++++--- r11k/puppetmodule/forge.py | 22 ++++++++++- r11k/puppetmodule/git.py | 19 +++++++++- r11k/util.py | 2 + r11k/version.py | 2 + setup.cfg | 2 +- tests/test_forge.py | 4 +- 17 files changed, 291 insertions(+), 68 deletions(-) diff --git a/r11k/__main__.py b/r11k/__main__.py index ff9e76f..5742656 100644 --- a/r11k/__main__.py +++ b/r11k/__main__.py @@ -1,3 +1,5 @@ +"""Interactive entry point.""" + from r11k.main import __main import sys diff --git a/r11k/cache.py b/r11k/cache.py index 3044e83..a0ebf6c 100644 --- a/r11k/cache.py +++ b/r11k/cache.py @@ -1,4 +1,7 @@ +"""Simple Key-Value cache backed by disk.""" + import os + import json import atexit import hashlib @@ -7,7 +10,7 @@ from r11k import util from typing import Any, Optional -def encode(string: str) -> str: +def _encode(string: str) -> str: """Calculate cheksum string for string.""" return hashlib.sha256(string.encode('UTF-8')).hexdigest() @@ -16,7 +19,13 @@ class KeyValueStore: """ A simple key-value store. - Serializes its data to disk as one json file for each key. If the data is + Serializes its data to disk as one json file for each key. + Serialization to disk happens upon program end, through an + `atexit` handler. + + ### Example + + If the data is >>> { 'a': 1, 'b': { 'c': 3 } } then the data is stored as: @@ -58,7 +67,7 @@ class KeyValueStore: def path(self, key: str) -> str: """Return filesystem path where this key would be stored.""" - key = encode(key) + key = _encode(key) return os.path.join(self._path, key) def get(self, key: str) -> Optional[Any]: @@ -67,7 +76,7 @@ class KeyValueStore: Lazily loads it from disk. Return None if not found. """ - key = encode(key) + key = _encode(key) if it := self.data.get(key): return it else: @@ -82,6 +91,6 @@ class KeyValueStore: def put(self, key: str, value: Any) -> None: """Store the given value under key.""" - _key = encode(key) + _key = _encode(key) self.data[_key] = value self._index[key] = _key diff --git a/r11k/config.py b/r11k/config.py index 726bdc3..5ef459a 100644 --- a/r11k/config.py +++ b/r11k/config.py @@ -1,3 +1,10 @@ +""" +R11K configuration and global "singletons". + +Explicitly designed to be easy to substitute for another configuration +in tests. +""" + import os.path from typing import ( Optional @@ -62,8 +69,10 @@ class Config: @http_ttl.setter def http_ttl(self, value: int) -> None: + """Save a new TTL value, and forwards it to `self.httpcache.default_ttl`.""" self._http_ttl = value self.httpcache.default_ttl = value config = Config() +"""Default configuration instance when none has been defined.""" diff --git a/r11k/forge.py b/r11k/forge.py index d59b5aa..350c26d 100644 --- a/r11k/forge.py +++ b/r11k/forge.py @@ -1,3 +1,14 @@ +""" +Python instances of all objects returned by the Puppet Forge API. + +This code could have been generated from their OpenAPI specification, +but the Python tools for it doesn't feel mature enough. Due to that +the names below are a bit arbitrary. + +Full documentation about each field can be found at +https://forgeapi.puppet.com/ +""" + from typing import ( Any, Literal, @@ -10,28 +21,20 @@ from semver import VersionInfo from r11k.puppet import PuppetMetadata -def parse_date(date: str) -> datetime: +def _parse_date(date: str) -> datetime: """Parse date from the format puppet forge uses (almost ISO8601).""" return datetime.strptime(date, "%Y-%m-%d %H:%M:%S %z") class Superseeded(TypedDict): # pragma: no cover - """ - Info about which modulesuperseeded the current module. - - https://forgeapi.puppet.com/#tag/Module-Operations/operation/getModule - """ + """Info about which modulesuperseeded the current module.""" uri: str slug: str class Owner(TypedDict): # pragma: no cover - """ - Owner of a given puppet module. - - https://forgeapi.puppet.com/#tag/Module-Operations/operation/getModule - """ + """Owner of a given puppet module.""" uri: str slug: str @@ -40,7 +43,11 @@ class Owner(TypedDict): # pragma: no cover class Release: - """Release info about a puppet module.""" + """ + Release info about a puppet module. + + Returned by `/v3/releases` (the list and search endpoint). + """ def __init__(self, uri: str, @@ -67,17 +74,21 @@ class Release: if isinstance(created_at, datetime): self.created_at = created_at else: - self.created_at = parse_date(created_at) + self.created_at = _parse_date(created_at) self.deleted_at: Optional[datetime] = None if isinstance(deleted_at, datetime): self.deleted_at = deleted_at elif isinstance(deleted_at, str): - self.deleted_at = parse_date(deleted_at) + self.deleted_at = _parse_date(deleted_at) class ForgeModule: - """Data about a puppet module.""" + """ + Data about a puppet module. + + Return value of `/v3/modules` (the list and search endpoint). + """ def __init__(self, uri: str, @@ -93,12 +104,17 @@ class ForgeModule: if isinstance(deprecated_at, datetime): self.deprecated_at = deprecated_at elif isinstance(deprecated_at, str): - self.deprecated_at = parse_date(deprecated_at) + self.deprecated_at = _parse_date(deprecated_at) self.owner = owner class CurrentRelease(Release): - """Extended release info about a puppet module.""" + """ + Extended release info about a puppet module. + + Superset of Releases for the currently active version (or an older + if explicitly specified). + """ def __init__(self, uri: str, @@ -158,7 +174,7 @@ class CurrentRelease(Release): if isinstance(updated_at, datetime): self.updated_at = updated_at else: - self.updated_at = parse_date(updated_at) + self.updated_at = _parse_date(updated_at) self.deleted_for = deleted_for @@ -166,7 +182,9 @@ class FullForgeModule(ForgeModule): """ Extended data about a puppet module. - https://forgeapi.puppet.com/#tag/Module-Operations/operation/getModule + The extended superset when only checking a single module. + + Return value of `/v3/modules/{module_slug}`. """ def __init__(self, @@ -198,13 +216,13 @@ class FullForgeModule(ForgeModule): if isinstance(created_at, datetime): self.created_at = created_at else: - self.created_at = parse_date(created_at) + self.created_at = _parse_date(created_at) self.update_at: datetime if isinstance(updated_at, datetime): self.updated_at = updated_at else: - self.updated_at = parse_date(updated_at) + self.updated_at = _parse_date(updated_at) self.deprecated_for = deprecated_for self.superseded_by = superseded_by diff --git a/r11k/gitutil.py b/r11k/gitutil.py index 5da1b25..5f6ccc6 100644 --- a/r11k/gitutil.py +++ b/r11k/gitutil.py @@ -1,3 +1,5 @@ +"""Various minor utilities for git repos.""" + from datetime import datetime import os import os.path @@ -11,7 +13,11 @@ import git def repo_last_fetch(repo: Repo) -> datetime: - """When was a fetch last perfermed on this repo.""" + """ + When was a fetch last perfermed on this repo. + + :raises: `FileNotFoundError` if no suitable HEAD could be found. + """ files = [ os.path.join(repo.git_dir, 'FETCH_HEAD'), os.path.join(repo.git_dir, 'HEAD'), diff --git a/r11k/httpcache.py b/r11k/httpcache.py index ddb00cd..47b06ed 100644 --- a/r11k/httpcache.py +++ b/r11k/httpcache.py @@ -1,3 +1,5 @@ +"""Extension of `r11k.cache.KeyValueStore`. Should possibly be merged.""" + import os import os.path from datetime import datetime diff --git a/r11k/interval.py b/r11k/interval.py index 86f449d..c51ffc3 100644 --- a/r11k/interval.py +++ b/r11k/interval.py @@ -1,3 +1,10 @@ +""" +Intervals of versions. + +Also includes operations for manipulating and comparing those +intervals. +""" + import operator from typing import ( @@ -15,17 +22,28 @@ from .version import ( class Interval: """ - Upper and lower bound of a version interval. + An interval of versions. Keeps information about the endpoints, along with who imposed that constraint. + ### Examples and operators + Two intervals are equal (with regards to `==`) only if they are + identical, including their `max_by` and `min_by` fields. + + Intervals supports the `in` operator for `VersionInfo` objects, + returning if the given version is within the intervals bounds. + :param minimi: Lower bound of the interval, default to 0.0.0 :param maximi: Upper bound of the interval, defaults to (2⁶³).0.0 (which is basically infinity) :param min_cmp: :param max_cmp: - :param by: + :param by: Fallback for both `min_by` and `max_by` if either isn't given. + :param min_by: + :param max_by: + + :raises: `ValueError` if `self.minimi > self.maximi`. """ def __init__(self, @@ -45,12 +63,26 @@ class Interval: raise ValueError("Only '<' and '<=' are valid for max_cmp") self.minimi: VersionInfo = minimi + """Lower bound for interval.""" self.maximi: VersionInfo = maximi - self.min_cmp = min_cmp - self.max_cmp = max_cmp + """Upper bound for interval.""" + self.min_cmp: Comp = min_cmp + """Comparison operator for the lower bound, *must* be `>` or `≥`.""" + self.max_cmp: Comp = max_cmp + """Comparison operator for the upper bound, *must* be `<` or `≤`.""" self.min_by: str = min_by or by or "" + """ + A description of who imposed the lower constraint in this interval. + + Used to later inspect why this interval looks like it does. + Retrival of this field should only be for display to the user, + and no assumptions should be made about the format. It will + however probably either be the name of a Puppet module, or a + comma separated list of them. + """ self.max_by: str = max_by or by or "" + """Like `min_by`, but for the upper bound.""" if self.minimi > self.maximi: raise ValueError("The end of an interval must be greater (or equal) to the start") @@ -115,7 +147,12 @@ def __join_opt_strings(*strings: Optional[str]) -> Optional[str]: def overlaps(a: Interval, b: Interval) -> bool: - """Is the two intervals overlapping.""" + """ + Check if interval a overlaps interval b. + + The case where `a.maximi == b.minimi` (or vice versa) only + overlaps when both intervals are inclusive in that point. + """ # Case 1 Case 2 Case 3 Case 4 # [---) [---] [---) [---] # (---] (---] [---] [---] @@ -147,7 +184,11 @@ def overlaps(a: Interval, b: Interval) -> bool: def intersect(a: Interval, b: Interval) -> Interval: - """Return the intersection of the two given intervals.""" + """ + Return the intersection of the two given intervals. + + :raises: `ValueError` if the two intervals don't overlap. + """ minimi: VersionInfo maximi: VersionInfo min_by: Optional[str] @@ -223,13 +264,13 @@ def parse_interval(s: str, """ Parse a string as an interval. - :param s: String to parse, should be on form [<start>, <end>], + :param s: String to parse, should be on form `[<start>, <end>]`, where the brackes can be either hard or soft (for closed and open intervals), and start and end must both be parsable by semver.VersionInfo.parse. - :param by: Passed verbatim to Intervals constructor - :param min_by: Passed verbatim to Intervals constructor - :param max_by: Passed verbatim to Intervals constructor + :param by: Passed verbatim to `Interval`s constructor + :param min_by: Passed verbatim to `Interval`s constructor + :param max_by: Passed verbatim to `Interval`s constructor """ min_cmp: Comp max_cmp: Comp diff --git a/r11k/main.py b/r11k/main.py index 95127cf..cee5c43 100644 --- a/r11k/main.py +++ b/r11k/main.py @@ -1,4 +1,13 @@ -# https://puppet.com/docs/puppet/7/modules_metadata.html +""" +Entry point and dependency resolution. + +This module currently does to much. It's simultaneously +- the entry point of R11K +- The specification of our puppetfile.yaml format + - The thing which publishes our environments +- the dependency resolver +- and probably something more +""" from dataclasses import dataclass, field from functools import reduce diff --git a/r11k/puppet.py b/r11k/puppet.py index 8862395..bf85823 100644 --- a/r11k/puppet.py +++ b/r11k/puppet.py @@ -1,3 +1,20 @@ +""" +Python mapping of Puppet's metadata files. + +Each Puppet module *ought* to contain a `metadata.json` file, and +*all* modules on the Puppet Forge has a metadata file, including ways +to retrieve only them through the Forge's API. + +Puppets metadata format is [documented here][metadata], and is mapped +almost directly here. Worth noting that items are mapped to python +types. + +All constructors take any extra keyword arguments to future proof +against future versions of Puppets metadata files. + +[metadata]: https://puppet.com/docs/puppet/7/modules_metadata.html +""" + from typing import ( Any, Optional, @@ -13,8 +30,8 @@ class DependencySpec: # pragma: no cover """ A single dependency of a puppet module. - name is (probly) a the forge name - version_requirement is a string on the form ">= 4.1.0 < 8.0.0" + :param name: is (probly) a the Forge name + :param version_requirement: is a string on the form `>= 4.1.0 < 8.0.0" """ def __init__(self, name: str, version_requirement: str): @@ -45,10 +62,9 @@ class PuppetMetadata: """ Metadata of a puppet module. - Directly maps to the metadata.json file described [here][1], - which SHOULD be present in all puppet modules, and also accessible - through the forge. - [1]: https://puppet.com/docs/puppet/7/modules_metadata.html + The parameters which are unions between a simple Python type, and + a fully realized type *will* get parsed into the fully realized + type in the constructor. """ def __init__(self, @@ -65,23 +81,45 @@ class PuppetMetadata: operatingsystem_support: Optional[list[Union[dict, OSSupport]]] = None, tags: Optional[list[str]] = None, **_: Any): # pragma: no cover - self.name = name - self.version = version - self.author = author - self.license = license - self.summary = summary - self.source = source + self.name: str = name + """ + Concatenation of owner's Forge username with the modules + name, on the form *forge username*-*module name*. + """ + self.version: str = version + """Semver formatted string of version.""" + self.author: str = author + """Arbitrarly formatted name of author.""" + self.license: str = license + """Software License of module. + + *Should* be present in [SPDX's list of licenses](https://spdx.org/licenses/) + """ + self.summary: str = summary + """One-line summary of module.""" + self.source: str = source + """URL to source code of module.""" self.dependencies: list[DependencySpec] \ = [d if isinstance(d, DependencySpec) else DependencySpec(**d) for d in dependencies] + """List of other Puppet modules this module dependes on.""" self.requirements: list[DependencySpec] \ = [d if isinstance(d, DependencySpec) else DependencySpec(**d) for d in requirements] - self.project_page = project_page - self.issues_url = issues_url - self.operatingsystem_support: Optional[list[OSSupport]] = None + """List of external requirements. + + This includes Puppet version.""" + self.project_page: Optional[str] = project_page + """URL to project page of module.""" + self.issues_url: Optional[str] = issues_url + """URL to where issues can be submitted.""" + self.operatingsystem_support: Optional[list[OSSupport]] + """List of which operating systems this module is "compatible" with.""" if operatingsystem_support: self.operatingsystem_support \ = [x if isinstance(x, OSSupport) else OSSupport(**x) for x in operatingsystem_support] - self.tags = tags + else: + self.operatingsystem_support = None + self.tags: Optional[list[str]] = tags + """Keywords for module discovery.""" diff --git a/r11k/puppetmodule/__init__.py b/r11k/puppetmodule/__init__.py index 24d9b53..f4b7c3d 100644 --- a/r11k/puppetmodule/__init__.py +++ b/r11k/puppetmodule/__init__.py @@ -1,7 +1,16 @@ """ -PuppetModule base is the abstract base class for any source of a puppet module. +Datatypes for requested Puppet modules. -Each other module is an implementation of a specific backend. +An abstract base class which implements our expected interface is +available in `r11k.puppetmodule.base`. Every other submodule here +should be an implementation, providing a source for where to get +Puppet modules. + +This is what is stored in puppetfile.yaml + +The implementations are in +- `r11k.puppetmodule.git` +- `r11k.puppetmodule.forge` """ from .base import PuppetModule @@ -10,7 +19,12 @@ from .forge import ForgePuppetModule def parse_module_dict(d: dict) -> PuppetModule: - """Parse dict describing module into module object.""" + """ + Parse dict describing module into module object. + + Currently, it becomes a `GitPuppetModule` if the `git` key is + present, and a `ForgePuppetModule` otherwise. + """ if d.get('git'): return GitPuppetModule(**d) else: diff --git a/r11k/puppetmodule/base.py b/r11k/puppetmodule/base.py index 6fa7090..99beb66 100644 --- a/r11k/puppetmodule/base.py +++ b/r11k/puppetmodule/base.py @@ -1,3 +1,12 @@ +""" +Abstract base class for Puppet module sources. + +Puppet modules can come from many places. Here we provide a common +interface for how the rest of the program should be able to interact +with them. A few implementation parts are here, for things that will +be common among almost all module providers. +""" + from typing import ( Optional, ) @@ -8,12 +17,24 @@ import r11k.config class PuppetModule: r""" - Metadata about wanted puppet module. + Abstract base class for all puppet modules. + + Each implementation *must* implement the following: + - `_fetch_metadata` + - `publish` + - `versions` - This is what is stored in puppetfile.yaml + ##### Protected fields + ###### `_fetch_metadata` + A method which retrieves the metadata from *somewhere*. The + property `metadata` by default caches this information. - :param name: forge-name (`/^(<owner>\w*)-(<modulename>\w*)-(<version>.*)/`) - :param version: semver-version | git-version + :param name: Name of the module. Further meaning depends on the + implementation. + :param version: Version to use. Valid values depend on the + implementation. + :param config: Runtime configuration which may be used by + implementations to aid them in retrieving a module. """ # pragma: no cover @@ -30,7 +51,14 @@ class PuppetModule: @property def metadata(self) -> PuppetMetadata: - """Return metadata object for this module, fetching it if needed.""" + """ + Property containing the modules metadata. + + The metadata almost always needs to be fetched from an + external source. Each implementation must implement + `_fetch_metadata` which solves this, while this property + ensures that `_fetch_metadata` is only called once. + """ if not self._metadata: self._metadata = self._fetch_metadata() return self._metadata @@ -44,11 +72,17 @@ class PuppetModule: Publish this module to the given path. TODO should path include the module name? + + :raises: `NotImplementedError` """ raise NotImplementedError() def versions(self) -> list[VersionInfo]: # pragma: no cover - """List available versions of this module.""" + """ + List available versions of this module. + + :raises: `NotImplementedError` + """ raise NotImplementedError() def latest(self) -> VersionInfo: diff --git a/r11k/puppetmodule/forge.py b/r11k/puppetmodule/forge.py index 80e871b..5332f1e 100644 --- a/r11k/puppetmodule/forge.py +++ b/r11k/puppetmodule/forge.py @@ -1,3 +1,18 @@ +""" +Modules form the [Puppet Forge][forge]. + +The Forge notes a few installation methods, where we (currently) +aren't listed. The manual install method + +```sh +puppet module install puppetlabs-stdlib --version 8.4.0 +``` +maps unto us as +>>> ForgePuppetModule(name='puppetlabs-stdlib', version='8.4.0') + +[forge]: https://forge.puppet.com/modules/puppetlabs/stdlib +""" + import logging import os.path import hashlib @@ -20,7 +35,12 @@ logger = logging.getLogger(__name__) class ForgePuppetModule(PuppetModule): - """Puppet module backed by the Puppet Forge.""" + """ + Puppet module backed by the Puppet Forge. + + :param name: Module name, as noted on the Forge for installation. + :param version: Semver formatted string, as per the Puppet Forge + """ def _fetch_metadata(self) -> PuppetMetadata: """Return metadata for this module.""" diff --git a/r11k/puppetmodule/git.py b/r11k/puppetmodule/git.py index 8041e1f..6e2eeef 100644 --- a/r11k/puppetmodule/git.py +++ b/r11k/puppetmodule/git.py @@ -1,3 +1,5 @@ +"""Puppet modules backed by git repos.""" + from datetime import datetime import json import logging @@ -26,7 +28,22 @@ logger = logging.getLogger(__name__) class GitPuppetModule(PuppetModule): - """Puppet module backed by git repo.""" + """ + Puppet module backed by git repo. + + :param name: If the given module dosen't provide any metadata, + then this becomes the name of the published directory + for module (and must therefor match what the module + is written to be published as). If the module + contains metadata.json, the the name will be changed + to that, but a warning will be issued on mismatches. + :param git: URL to a git repo containing a Puppet module + :param version: Any valid git reference, such as a + tag, + branch, + commit, + etc... + """ # TODO should latest be overridden for git to always return HEAD? diff --git a/r11k/util.py b/r11k/util.py index c06f479..bf19410 100644 --- a/r11k/util.py +++ b/r11k/util.py @@ -1,3 +1,5 @@ +"""Minor utilities which didn't fit anywhere else.""" + import logging import os import requests diff --git a/r11k/version.py b/r11k/version.py index feb3763..c6224a4 100644 --- a/r11k/version.py +++ b/r11k/version.py @@ -1,3 +1,5 @@ +"""Extension of `semver` for parsing Puppet version specifications.""" + import operator import re diff --git a/setup.cfg b/setup.cfg index b6c7fea..e9cf3f9 100644 --- a/setup.cfg +++ b/setup.cfg @@ -46,7 +46,7 @@ max-line-length = 100 ignore = W503 [pydocstyle] -ignore = D100,D105,D107,D203,D212 +ignore = D105,D107,D203,D212 [mypy] ignore_missing_imports = True diff --git a/tests/test_forge.py b/tests/test_forge.py index 34aa3cd..e106bb7 100644 --- a/tests/test_forge.py +++ b/tests/test_forge.py @@ -1,4 +1,4 @@ -from r11k.forge import parse_date +from r11k.forge import _parse_date from datetime import datetime, timezone, timedelta @@ -7,4 +7,4 @@ def test_parse_date(): dt = datetime(year=2022, month=9, day=16, hour=23, minute=35, second=7, tzinfo=timezone(timedelta(hours=2))) - assert dt == parse_date('2022-09-16 23:35:07 +02:00') + assert dt == _parse_date('2022-09-16 23:35:07 +02:00') -- GitLab