diff --git a/.claude/rules/copier-template.md b/.claude/rules/copier-template.md index 096a85f..61277e6 100644 --- a/.claude/rules/copier-template.md +++ b/.claude/rules/copier-template.md @@ -13,7 +13,10 @@ the next `copier update` either reverts them or turns them into a merge conflict. Add project specific build steps to `TaskfileCustom.yaml` instead - note the -`.yaml` spelling, since only that one is included. Project specific agent +`.yaml` spelling, since only that one is included. A GitLab pipeline takes +project specific jobs from `.gitlab-ci-custom.yml` the same way, included when +that file exists; it can add to a templated job but not redefine one, and +`.github/workflows/` has no equivalent hook. Project specific agent instructions belong in `CLAUDE.md`, which the template never writes and which is read alongside these rules. Personal tool permissions belong in `.claude/settings.local.json`, which is git-ignored. @@ -49,6 +52,14 @@ silence the rule for code nobody has looked at. Report it with the `template-feedback` skill too, since a rule that is wrong here is usually wrong in other projects as well. +One class of that is already settled and needs neither a human nor a report: +`PLR0913` and `PLR0917` on a signature whose arity a framework fixes rather +than the author - a plugin constructor with one argument per `PluginParameter`, +a Click command with one per option. Suppress both on the `def`, with the +reason above it. Neither escape ruff suggests is available, since keyword-only +arguments answer `PLR0917` but not `PLR0913`, and folding the arguments into +one object breaks the one-argument-per-option mapping the framework relies on. + ## Every user visible change gets a changelog entry `CHANGELOG.md` follows [Keep a Changelog](https://keepachangelog.com/). Add an diff --git a/.claude/skills/plugin-documentation/SKILL.md b/.claude/skills/plugin-documentation/SKILL.md index 4ef700e..d4b5fbd 100644 --- a/.claude/skills/plugin-documentation/SKILL.md +++ b/.claude/skills/plugin-documentation/SKILL.md @@ -11,8 +11,8 @@ This governs the text a user reads inside eccenca Corporate Memory: the `label`, Python docstrings, error messages or identifiers. Those are read by developers and follow the ordinary code conventions. -`documentation` is rendered as Markdown, so backticks, `**bold**` and links all -work. `description` is a single line shown next to the task in the task list. +`documentation` is rendered as Markdown, so backticks, `**bold**`, links, `##` +headings and lists all work. `description` is a single line shown next to the task in the task list. ## Ground every claim in the code @@ -66,6 +66,18 @@ lines. Do not pad a simple task to match a complex one, and do not cut a complex one down to match a simple one. A task with three mutually exclusive modes genuinely needs more words than one with a single input port. +Mark the beats by length, not by taste. While the block is short - which for +most tasks it is - write it as one run of paragraphs and let the beats follow +each other unannounced. Once it passes roughly twenty lines, give each beat a +`##` heading named after it, so a reader scans for the beat they need instead of +reading until they find it. The caveats beat is the one that stops working as +prose first, and reads best as a list with the claim in bold at the front of +each item. + +Letting the length decide is what keeps sibling tasks in one package looking +like one product. A user meets several of them in a workspace, and two +documentation styles there read as two different things. + ## Parameters The first sentence says what the parameter controls. Add further sentences only diff --git a/.claude/skills/plugin-implementation/SKILL.md b/.claude/skills/plugin-implementation/SKILL.md index 46c1872..7897ada 100644 --- a/.claude/skills/plugin-implementation/SKILL.md +++ b/.claude/skills/plugin-implementation/SKILL.md @@ -98,7 +98,12 @@ formatted string. The `logging` idiom `self.log.info("Fetched %s records", count)` raises `TypeError` at runtime, so an f-string is the form to use - the ruff rule `G004` (logging statement uses f-string) is in the `ignore` list accordingly. -The same gap is why `self.log` cannot be passed as a `logger` argument. +There is no `exception()` either, so a failure logged inside an `except` block +uses `error()` - ruff's `TRY400` asks for `logging.exception` there, and is in +the `ignore` list for the same reason as `G004`. Do not downgrade the call to +`warning()` to quieten the rule: that understates a failure that is usually +about to be re-raised. The same gap is why `self.log` cannot be passed as a +`logger` argument. ## The icon @@ -272,6 +277,54 @@ context.report.update( the work is finished shows a user nothing while the task is running, which is exactly when they are looking. +## The constructor decides the form, not the decorator list + +The `parameters=[...]` list in `@Plugin` reads as the order a user sees on the +task form. It is not. The descriptor is built by walking +`inspect.signature(__init__)` and looking each argument name up in the decorator +list, so the **constructor's argument order is the form's field order** and the +decorator list is only a lookup table for labels, descriptions and types. + +Two things follow: + +- Reordering the decorator list changes nothing a user can see, which is + unpleasant to debug because the file you edited is the one that looks + authoritative. Keep the two lists in the same order and reorder them together, + so the source reads the way the form does. +- A decorator entry whose name matches no constructor argument is **dropped + without an error**. A renamed argument therefore silently takes its label and + description with it, and the field appears bare rather than missing. + +Order the constructor the way the decision reads: a parameter goes before the +ones that depend on it, and above the flag that switches it on. Reordering is +safe because the framework instantiates by keyword - but check that nothing +constructs the class positionally first, which tests often do. + +## The one parameter name the task form claims + +Do not name a parameter `tags`. Every task dialog carries a "Tags" field for the +item's own tags, and that widget's value arrives on a plugin parameter of the +same name in place of the string the parameter declares, so saving the task - or +any other task action - fails with: + +```text +Invalid value for plugin parameter 'tags' of plugin '-'. +Got a nested object with the keys createdItems, selectedItems, but expected a string. +``` + +An empty tag list fails just as well, because the widget still posts both keys +as empty lists. Nothing local catches it: discovery succeeds, the descriptors +show the parameter present, labelled and typed as a string, and `task check` is +green - the failure exists only against a deployment, in the dialog. + +Name the parameter for what it carries - `notification_tags`, `issue_tags` - and +keep the user-visible wording in the `label`, which can still read "Tags". + +`label` and `description` are **not** claimed, although the dialog shows the +item's own fields under those names as well. `cmem-plugin-shapes`, +`cmem-plugin-metadata` and `cmem-plugin-vocab` all ship a parameter called +`label`, and `cmem-plugin-metadata` one called `description`. + ## A constructor with six or more parameters Ruff's `PLR0913` and `PLR0917` both complain about a long argument list, and a @@ -315,6 +368,57 @@ The value arrives as a `Password`; call `.decrypt()` only where it is used. Typing it as `str` puts the secret in plain text in the task configuration and in the project export. +An **empty secret field is still truthy**. A blank parameter arrives as a +`Password` wrapping an empty value, and `Password` defines neither `__bool__` +nor `__len__`, so the obvious guard never fires: + +```python +if self.api_key: # True even when the field was left blank + ... +``` + +A plugin whose behaviour depends on whether a secret was supplied takes the +wrong branch, and the failure surfaces much later - as a library error deep +inside whatever consumes the secret, or as a request sent with an empty +credential. Ask the question against the decrypted value, through a helper that +also accepts the plain `str` that a default value or a test supplies, and keep +the answer rather than the plaintext: + +```python +def _secret(value: Password | str) -> str: + return value if isinstance(value, str) else value.decrypt() + +self.signing = bool(_secret(private_key)) # keep the answer, not the secret +``` + +Tests hide this rather than catching it: a test passing a plain `str` exercises +the naive guard correctly, so the mistake only appears against a deployment, +where DataIntegration hands over a `Password`. + +## Validating an IRI the user types + +A knowledge graph IRI **is not necessarily a URL**. `urn:example:data` and +`urn:uuid:...` are ordinary ways to name a graph, and the store imports into and +queries one without complaint - so `validators.url()`, the obvious import, +rejects a graph that works. The trap is quiet: the check looks right, and the +resulting *Invalid value for parameter ...* never mentions that the scheme was +what failed. + +`GraphParameterType` covers the case where the user picks a graph. Hand-written +validation is for the ones they type - an ignore list, a target graph that does +not exist yet, a class or property filter - and it has to accept a URN as well: + +```python +URN = re.compile(r"^urn:[a-z0-9][a-z0-9-]{1,31}:\S+$", re.IGNORECASE) + +if not validators.url(iri) and not URN.match(iri): + raise ValueError(f"{iri} is neither a URL nor a URN.") +``` + +The namespace identifier is two to thirty-two characters starting with an +alphanumeric one, and the namespace specific string must be non-empty +(RFC 8141). + ## Custom parameter types A `PluginParameter` without an explicit `param_type` gets one derived from the @@ -378,6 +482,43 @@ class CollectionParameterType(StringParameterType): - Return a **stable order**. Sort at the end; do not deduplicate with `set()` after sorting, because that throws the ordering away again. +### A choice dropdown is not shown in its declared order + +`ChoiceParameterType` does the very thing that last bullet warns against, so +the `OrderedDict` it takes is **not** the order the user sees: `autocomplete()` +sorts by label and then returns `list(set(result))`, and `Autocompletion` is a +frozen dataclass, so the set reorders by hash bucket and discards the sort. +Python randomizes string hashing per process, so the dropdown comes out +differently after every restart of DataIntegration - which also means one run in +three looks plausible enough to miss. Nothing local shows it: the `OrderedDict` +is intact in the source and in the descriptors, and `task check` is green. + +So for a choice whose order carries meaning - priorities, log levels, sizes, +compression levels, conflict policies - subclass it until + is released: + +```python +class OrderedChoiceParameterType(ChoiceParameterType): + """A choice whose dropdown keeps the order the choices were declared in.""" + + def autocomplete( + self, + query_terms: list[str], + depend_on_parameter_values: list[Any], + context: PluginContext, + ) -> list[Autocompletion]: + """Return the choices matching all query terms, in their declared order.""" + terms = [term.lower() for term in query_terms] + return [ + Autocompletion(value=value, label=label) + for value, label in self.choice_list.items() + if all(term in label.lower() for term in terms) + ] +``` + +Numbering the labels does not rescue it - `(1)` to `(5)` only feeds the sort +that is thrown away. + ### The flags - `allow_only_autocompleted_values = True` makes the parameter a closed diff --git a/.claude/skills/plugin-testing/SKILL.md b/.claude/skills/plugin-testing/SKILL.md index 0c9de02..f36d5ab 100644 --- a/.claude/skills/plugin-testing/SKILL.md +++ b/.claude/skills/plugin-testing/SKILL.md @@ -91,6 +91,32 @@ broken plugin fails the assertion instead of the fixture. Never point a test at assets that happen to exist in the deployment: the next person runs the suite somewhere else. +The teardown runs **before** the test as well. A fixture whose asset has a +fixed id leaves it behind whenever a run crashes or is killed, and every later +run then fails in setup on the leftover rather than in the test it was meant to +exercise. Deleting first, tolerating an asset that is not there, makes the suite +self-healing: + +```python +@pytest.fixture +def project() -> Iterator[Client]: + """Provide an empty project, whatever the last run left behind.""" + client = get_client(PROJECT_ID) + client.projects.delete_item(PROJECT_ID, skip_if_missing=True) + client.projects.create_item(Project(name=PROJECT_ID)) + yield client + client.projects.delete_item(PROJECT_ID, skip_if_missing=True) +``` + +**Delete what the test created; never restore the whole store.** Snapshotting +the triple store before a test and importing the snapshot back afterwards does +restore the assets, and also reverts everything else written to the deployment +in between. It looks thorough rather than dangerous, which is why it gets +written - but integration tests commonly point at one shared instance, so the +blast radius is every other suite and every person using it, not the handful of +assets being undone. It is also slow: replacing such a snapshot with targeted +deletion took one suite from about 127 seconds to 22. + ## Finishing Run `task check:pytest` before finishing, and `task check` before considering diff --git a/.copier-answers.yml b/.copier-answers.yml index 3350c5e..96978ec 100644 --- a/.copier-answers.yml +++ b/.copier-answers.yml @@ -1,5 +1,5 @@ # Changes here will be overwritten by Copier -_commit: v9.7.0 +_commit: v9.9.0 _src_path: gh:eccenca/cmem-plugin-template author_mail: cmempy-developer@eccenca.com author_name: eccenca GmbH diff --git a/pyproject.toml b/pyproject.toml index e5a623f..80638ba 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -104,10 +104,12 @@ ignore = [ "ISC001", # single-line-implicit-string-concatenation "PD", # opinionated linting for pandas code "TRY003", # Avoid specifying long messages outside the exception class + "TRY400", # PluginLogger has no exception(), so error() is the only option ] [tool.ruff.lint.per-file-ignores] "tests/**/*.py" = [ "S101", # asserts allowed in tests "ARG", # unused function args (pytest fixtures) + "PLR2004", # in an assertion the expected value is the literal ]