Skip to content

Commit c123f19

Browse files
authored
Merge pull request #9 from eccenca/feature/updateTemplate
update to cmem-plugin-template v9.9.0
2 parents ec0e2e0 + 99f094c commit c123f19

8 files changed

Lines changed: 316 additions & 12 deletions

File tree

‎.claude/rules/copier-template.md‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,10 @@ the next `copier update` either reverts them or turns them into a merge
1313
conflict.
1414

1515
Add project specific build steps to `TaskfileCustom.yaml` instead - note the
16-
`.yaml` spelling, since only that one is included. Project specific agent
16+
`.yaml` spelling, since only that one is included. A GitLab pipeline takes
17+
project specific jobs from `.gitlab-ci-custom.yml` the same way, included when
18+
that file exists; it can add to a templated job but not redefine one, and
19+
`.github/workflows/` has no equivalent hook. Project specific agent
1720
instructions belong in `CLAUDE.md`, which the template never writes and which
1821
is read alongside these rules. Personal tool permissions belong in
1922
`.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
4952
`template-feedback` skill too, since a rule that is wrong here is usually wrong
5053
in other projects as well.
5154

55+
One class of that is already settled and needs neither a human nor a report:
56+
`PLR0913` and `PLR0917` on a signature whose arity a framework fixes rather
57+
than the author - a plugin constructor with one argument per `PluginParameter`,
58+
a Click command with one per option. Suppress both on the `def`, with the
59+
reason above it. Neither escape ruff suggests is available, since keyword-only
60+
arguments answer `PLR0917` but not `PLR0913`, and folding the arguments into
61+
one object breaks the one-argument-per-option mapping the framework relies on.
62+
5263
## Every user visible change gets a changelog entry
5364

5465
`CHANGELOG.md` follows [Keep a Changelog](https://keepachangelog.com/). Add an

‎.claude/skills/copier-update/SKILL.md‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,44 @@ the template actually changed, such as a bumped dependency constraint.
6363
Example files that were deleted after generation can reappear when the template
6464
changes them. Delete them again.
6565

66+
## What the merge will not tell you
67+
68+
A conflict is raised only where copier could not decide. The cases that cost
69+
the most time are the silent ones, and both directions have happened:
70+
71+
- **The template's new value is not applied.** The template set
72+
`cmem-cmemc` to `^25.3.0` in v7.3.0 and lowered it again to `>=24.2.0` in
73+
v8.0.0. Four repositories updated straight through that release and on to
74+
v9.6.1, and every one still carried the caret afterwards. No commit in them
75+
re-introduced it and no conflict was ever raised. It stayed harmless for a
76+
year, until click 8.5 began rejecting an empty colour that cmemc 25 passes
77+
through - and then several test suites failed at once with
78+
`Unknown color ''`.
79+
- **This project's value is replaced by the template's.** A project carrying its
80+
own `[tool.ruff.lint] ignore` and `per-file-ignores` entries lost them
81+
wholesale to the template's shorter lists, and the update appeared to add 409
82+
lint findings. They were not debt; the configuration that had silenced them
83+
was gone.
84+
85+
So do not read "no conflicts" as "nothing to check". After updating, look at
86+
`pyproject.toml` deliberately - at dependency constraints, and at every
87+
list-valued setting, because a list is replaced whole rather than merged
88+
entry by entry.
89+
90+
The reliable check is to render a throwaway project from the same answers and
91+
diff against it, which shows both what should have arrived and what should not
92+
have:
93+
94+
```bash
95+
copier copy --trust --defaults --vcs-ref <the tag you updated to> \
96+
-d project_slug=<slug> -d project_type=<plugin|generic> \
97+
gh:eccenca/cmem-plugin-template /tmp/reference
98+
diff /tmp/reference/pyproject.toml pyproject.toml
99+
```
100+
101+
Treat a sudden jump in lint findings as suspected configuration loss rather
102+
than real debt, and check that first.
103+
66104
## After updating
67105

68106
```bash

‎.claude/skills/plugin-documentation/SKILL.md‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ This governs the text a user reads inside eccenca Corporate Memory: the `label`,
1111
Python docstrings, error messages or identifiers. Those are read by developers
1212
and follow the ordinary code conventions.
1313

14-
`documentation` is rendered as Markdown, so backticks, `**bold**` and links all
15-
work. `description` is a single line shown next to the task in the task list.
14+
`documentation` is rendered as Markdown, so backticks, `**bold**`, links, `##`
15+
headings and lists all work. `description` is a single line shown next to the task in the task list.
1616

1717
## Ground every claim in the code
1818

@@ -66,6 +66,18 @@ lines. Do not pad a simple task to match a complex one, and do not cut a
6666
complex one down to match a simple one. A task with three mutually exclusive
6767
modes genuinely needs more words than one with a single input port.
6868

69+
Mark the beats by length, not by taste. While the block is short - which for
70+
most tasks it is - write it as one run of paragraphs and let the beats follow
71+
each other unannounced. Once it passes roughly twenty lines, give each beat a
72+
`##` heading named after it, so a reader scans for the beat they need instead of
73+
reading until they find it. The caveats beat is the one that stops working as
74+
prose first, and reads best as a list with the claim in bold at the front of
75+
each item.
76+
77+
Letting the length decide is what keeps sibling tasks in one package looking
78+
like one product. A user meets several of them in a workspace, and two
79+
documentation styles there read as two different things.
80+
6981
## Parameters
7082

7183
The first sentence says what the parameter controls. Add further sentences only

‎.claude/skills/plugin-implementation/SKILL.md‎

Lines changed: 217 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,39 @@ use `setup_cmempy_user_access()`. Plenty of existing plugins still call it -
4646
that is legacy, not a pattern to copy. `cmem-plugin-base` continues to depend
4747
on `cmem-cmempy` transitively, which does not make it available for new code.
4848

49+
## The plugin identifier
50+
51+
Set `plugin_id` explicitly on every `@Plugin`:
52+
53+
```python
54+
@Plugin(
55+
label="Create thing",
56+
plugin_id="cmem_plugin_example-CreateThing",
57+
...
58+
)
59+
```
60+
61+
Left unset, it is generated from where the code happens to sit -
62+
`generate_id(module_path + "-" + ClassName)`, so a task in
63+
`cmem_plugin_example/tasks/create.py` is called
64+
`cmem_plugin_example-tasks-create-CreateThing`. A workflow references a plugin
65+
by that identifier, so moving the module or renaming the class changes the
66+
identity of an already deployed task and orphans every workflow task built on
67+
it. That turns an ordinary refactoring into a breaking change, without an error
68+
anywhere.
69+
70+
Name it `<package_dir>-<Name>`. The prefix carries the weight - it keeps the
71+
identifier distinct from every other plugin in the same workspace - and
72+
`<Name>` is the class name, or a shortened form where the class repeats
73+
something the package name already says: `cmem_plugin_ssh-Download` for
74+
`DownloadFiles`. An identifier you supply bypasses `generate_id()` and is
75+
therefore never sanitised, so keep it to `[a-zA-Z0-9_-]` yourself.
76+
77+
**For a task that is already deployed, write down the identifier it generates
78+
today, verbatim** - work it out with the formula above rather than inventing a
79+
tidier one. The point of setting the field is to stop the identifier moving; a
80+
first commit that changes it breaks exactly what it is there to protect.
81+
4982
## Logging
5083

5184
The base class already provides a logger as `self.log`. Use it:
@@ -65,7 +98,12 @@ formatted string. The `logging` idiom
6598
`self.log.info("Fetched %s records", count)` raises `TypeError` at runtime, so
6699
an f-string is the form to use - the ruff rule
67100
`G004` (logging statement uses f-string) is in the `ignore` list accordingly.
68-
The same gap is why `self.log` cannot be passed as a `logger` argument.
101+
There is no `exception()` either, so a failure logged inside an `except` block
102+
uses `error()` - ruff's `TRY400` asks for `logging.exception` there, and is in
103+
the `ignore` list for the same reason as `G004`. Do not downgrade the call to
104+
`warning()` to quieten the rule: that understates a failure that is usually
105+
about to be re-raised. The same gap is why `self.log` cannot be passed as a
106+
`logger` argument.
69107

70108
## The icon
71109

@@ -156,6 +194,45 @@ same thing - a parameter here versus a connected input - rather than accepting
156194
both and picking one at runtime. A boolean or an enum parameter drives the same
157195
pattern with an `if`/`match` in place of the empty-string check.
158196

197+
### What `execute()` receives
198+
199+
The declaration settles what `inputs` can hold, and the two directions are not
200+
symmetric.
201+
202+
Under `FixedNumberOfInputs` it never carries **more** than the declared number
203+
of ports. The workflow editor offers exactly the handlers a task declares, so a
204+
one-port task cannot be connected to two upstream tasks. Code that warns about
205+
or trims a surplus is unreachable from a workflow, and it especially must not
206+
be mentioned in the task's `documentation`: telling a user to avoid a state they
207+
cannot configure is worse than saying nothing. Calling `execute()` directly from
208+
Python bypasses the editor entirely, which is why a test - or a code review
209+
reading the body alone - can "reproduce" the surplus and make a non-issue look
210+
like a defect.
211+
212+
It can carry **fewer**. A declared port the workflow leaves unconnected is
213+
absent from the sequence rather than arriving as an empty `Entities`, and
214+
DataIntegration runs the workflow anyway, so `inputs[0]` raises `IndexError`
215+
on a task whose one port nobody wired. That case is real, and each task decides
216+
what it means:
217+
218+
```python
219+
entities = list(inputs[index].entities) if index < len(inputs) else []
220+
```
221+
222+
reads an unconnected port as an empty one - the right call when input is an
223+
optional addition to what parameters already supply. Where the input is the
224+
whole point, fail with a message naming the cause instead:
225+
226+
```python
227+
if not inputs:
228+
raise ValueError("No input was given.")
229+
```
230+
231+
Both are used across the fleet. With several declared ports, do not additionally
232+
assume that a partly connected task keeps each remaining port at its declared
233+
index; drive the loop from `len(inputs)` or accept only what you can identify
234+
from the entities themselves.
235+
159236
## Honouring cancellation
160237

161238
A long-running task must stop when the user cancels the workflow. Check the
@@ -173,7 +250,9 @@ for entity in inputs[0].entities:
173250

174251
The `suppress(AttributeError)` is required, not defensive noise:
175252
`context.workflow` is absent in some contexts - notably the test contexts - and
176-
an unguarded check raises there while working in production.
253+
an unguarded check raises there while working in production. The `inputs[0]` in
254+
front of it is shorthand for a task whose port is connected; see *What
255+
`execute()` receives* for the case where it is not.
177256

178257
## Reporting progress
179258

@@ -198,6 +277,54 @@ context.report.update(
198277
the work is finished shows a user nothing while the task is running, which is
199278
exactly when they are looking.
200279

280+
## The constructor decides the form, not the decorator list
281+
282+
The `parameters=[...]` list in `@Plugin` reads as the order a user sees on the
283+
task form. It is not. The descriptor is built by walking
284+
`inspect.signature(__init__)` and looking each argument name up in the decorator
285+
list, so the **constructor's argument order is the form's field order** and the
286+
decorator list is only a lookup table for labels, descriptions and types.
287+
288+
Two things follow:
289+
290+
- Reordering the decorator list changes nothing a user can see, which is
291+
unpleasant to debug because the file you edited is the one that looks
292+
authoritative. Keep the two lists in the same order and reorder them together,
293+
so the source reads the way the form does.
294+
- A decorator entry whose name matches no constructor argument is **dropped
295+
without an error**. A renamed argument therefore silently takes its label and
296+
description with it, and the field appears bare rather than missing.
297+
298+
Order the constructor the way the decision reads: a parameter goes before the
299+
ones that depend on it, and above the flag that switches it on. Reordering is
300+
safe because the framework instantiates by keyword - but check that nothing
301+
constructs the class positionally first, which tests often do.
302+
303+
## The one parameter name the task form claims
304+
305+
Do not name a parameter `tags`. Every task dialog carries a "Tags" field for the
306+
item's own tags, and that widget's value arrives on a plugin parameter of the
307+
same name in place of the string the parameter declares, so saving the task - or
308+
any other task action - fails with:
309+
310+
```text
311+
Invalid value for plugin parameter 'tags' of plugin '<package>-<Task>'.
312+
Got a nested object with the keys createdItems, selectedItems, but expected a string.
313+
```
314+
315+
An empty tag list fails just as well, because the widget still posts both keys
316+
as empty lists. Nothing local catches it: discovery succeeds, the descriptors
317+
show the parameter present, labelled and typed as a string, and `task check` is
318+
green - the failure exists only against a deployment, in the dialog.
319+
320+
Name the parameter for what it carries - `notification_tags`, `issue_tags` - and
321+
keep the user-visible wording in the `label`, which can still read "Tags".
322+
323+
`label` and `description` are **not** claimed, although the dialog shows the
324+
item's own fields under those names as well. `cmem-plugin-shapes`,
325+
`cmem-plugin-metadata` and `cmem-plugin-vocab` all ship a parameter called
326+
`label`, and `cmem-plugin-metadata` one called `description`.
327+
201328
## A constructor with six or more parameters
202329

203330
Ruff's `PLR0913` and `PLR0917` both complain about a long argument list, and a
@@ -241,6 +368,57 @@ The value arrives as a `Password`; call `.decrypt()` only where it is used.
241368
Typing it as `str` puts the secret in plain text in the task configuration and
242369
in the project export.
243370

371+
An **empty secret field is still truthy**. A blank parameter arrives as a
372+
`Password` wrapping an empty value, and `Password` defines neither `__bool__`
373+
nor `__len__`, so the obvious guard never fires:
374+
375+
```python
376+
if self.api_key: # True even when the field was left blank
377+
...
378+
```
379+
380+
A plugin whose behaviour depends on whether a secret was supplied takes the
381+
wrong branch, and the failure surfaces much later - as a library error deep
382+
inside whatever consumes the secret, or as a request sent with an empty
383+
credential. Ask the question against the decrypted value, through a helper that
384+
also accepts the plain `str` that a default value or a test supplies, and keep
385+
the answer rather than the plaintext:
386+
387+
```python
388+
def _secret(value: Password | str) -> str:
389+
return value if isinstance(value, str) else value.decrypt()
390+
391+
self.signing = bool(_secret(private_key)) # keep the answer, not the secret
392+
```
393+
394+
Tests hide this rather than catching it: a test passing a plain `str` exercises
395+
the naive guard correctly, so the mistake only appears against a deployment,
396+
where DataIntegration hands over a `Password`.
397+
398+
## Validating an IRI the user types
399+
400+
A knowledge graph IRI **is not necessarily a URL**. `urn:example:data` and
401+
`urn:uuid:...` are ordinary ways to name a graph, and the store imports into and
402+
queries one without complaint - so `validators.url()`, the obvious import,
403+
rejects a graph that works. The trap is quiet: the check looks right, and the
404+
resulting *Invalid value for parameter ...* never mentions that the scheme was
405+
what failed.
406+
407+
`GraphParameterType` covers the case where the user picks a graph. Hand-written
408+
validation is for the ones they type - an ignore list, a target graph that does
409+
not exist yet, a class or property filter - and it has to accept a URN as well:
410+
411+
```python
412+
URN = re.compile(r"^urn:[a-z0-9][a-z0-9-]{1,31}:\S+$", re.IGNORECASE)
413+
414+
if not validators.url(iri) and not URN.match(iri):
415+
raise ValueError(f"{iri} is neither a URL nor a URN.")
416+
```
417+
418+
The namespace identifier is two to thirty-two characters starting with an
419+
alphanumeric one, and the namespace specific string must be non-empty
420+
(RFC 8141).
421+
244422
## Custom parameter types
245423

246424
A `PluginParameter` without an explicit `param_type` gets one derived from the
@@ -304,6 +482,43 @@ class CollectionParameterType(StringParameterType):
304482
- Return a **stable order**. Sort at the end; do not deduplicate with `set()`
305483
after sorting, because that throws the ordering away again.
306484

485+
### A choice dropdown is not shown in its declared order
486+
487+
`ChoiceParameterType` does the very thing that last bullet warns against, so
488+
the `OrderedDict` it takes is **not** the order the user sees: `autocomplete()`
489+
sorts by label and then returns `list(set(result))`, and `Autocompletion` is a
490+
frozen dataclass, so the set reorders by hash bucket and discards the sort.
491+
Python randomizes string hashing per process, so the dropdown comes out
492+
differently after every restart of DataIntegration - which also means one run in
493+
three looks plausible enough to miss. Nothing local shows it: the `OrderedDict`
494+
is intact in the source and in the descriptors, and `task check` is green.
495+
496+
So for a choice whose order carries meaning - priorities, log levels, sizes,
497+
compression levels, conflict policies - subclass it until
498+
<https://github.com/eccenca/cmem-plugin-base/issues/49> is released:
499+
500+
```python
501+
class OrderedChoiceParameterType(ChoiceParameterType):
502+
"""A choice whose dropdown keeps the order the choices were declared in."""
503+
504+
def autocomplete(
505+
self,
506+
query_terms: list[str],
507+
depend_on_parameter_values: list[Any],
508+
context: PluginContext,
509+
) -> list[Autocompletion]:
510+
"""Return the choices matching all query terms, in their declared order."""
511+
terms = [term.lower() for term in query_terms]
512+
return [
513+
Autocompletion(value=value, label=label)
514+
for value, label in self.choice_list.items()
515+
if all(term in label.lower() for term in terms)
516+
]
517+
```
518+
519+
Numbering the labels does not rescue it - `(1)` to `(5)` only feeds the sort
520+
that is thrown away.
521+
307522
### The flags
308523

309524
- `allow_only_autocompleted_values = True` makes the parameter a closed

0 commit comments

Comments
 (0)