Skip to content

feat: declare the attribute contracts the CLI mixins rely on - #50

Open
blaipr wants to merge 1 commit into
ctrliq:mainfrom
blaipr:feat/type-the-cli-base-classes
Open

feat: declare the attribute contracts the CLI mixins rely on#50
blaipr wants to merge 1 commit into
ctrliq:mainfrom
blaipr:feat/type-the-cli-base-classes

Conversation

@blaipr

@blaipr blaipr commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

First substantive step on the typing work that #42 only set up the measurement for. It takes the CLI from 48 type diagnostics to 2, and the package from 136 to 82, without annotating a single function signature.

The reason so much falls out of so little: cli/custom.py builds its actions out of mixins. Launchable, HasStdout, AssociationMixin, RoleMixin and HasMonitor all read self.page, self.action and self.resource, and none of them declares any of the three, because the CustomAction they get mixed into supplies them. That contract lived only in the reader's head, and it accounted for 46 of the diagnostics on its own.

Each mixin now states it:

class Launchable:
    # Supplied by the CustomAction this is mixed into, which is why they are
    # annotations rather than assignments: they describe the contract without
    # creating class attributes that would shadow the real ones.
    page: 'api.pages.Page'
    action: str
    resource: str

Annotations without assignment create no class attribute, so there is no runtime change and nothing shadows what the subclasses set.

Three more, each a real finding rather than a checker being appeased:

CustomAction.action and .resource were @property methods raising NotImplementedError. Every subclass sets them as plain class attributes, action = 'launch', so the properties were never reached and they conflicted with the mixin declarations above. They are annotations now, which is what the subclasses actually satisfy. perform keeps the property, since subclasses genuinely override it as a method.

Two add_arguments overrides were narrower than what they replace. Launchable.add_arguments takes with_pk=True; BulkJobLaunch and AdhocCommandLaunch declared it without that parameter and then passed with_pk=False to the parent by hand. They now carry the parameter with a False default and pass it through, so the behaviour is identical and the signature no longer contradicts the base.

NotificationAssociateMixin.targets is not dict[str, list[str]]. The three literals suggest it is, and then six targets.update() calls add entries like ['credentials', None]. It is annotated as dict[str, list[str | None]], which is what it has always held.

Two diagnostics remain in cli/ and both are left on purpose:

  • cli/format.py:151, Cannot resolve imported module 'jq'. It is the formatting extra, imported inside the function that needs it. Installing the extra in the type-check environment resolves it; the code is correct as it stands.
  • cli/utils.py:29, command.name inside CustomRegistryMeta.registry. type.__subclasses__() is typed list[type], which says nothing about the name every command declares. Fixing it means a cast, and contorting a metaclass to satisfy a checker is worse than the diagnostic.

The remaining 80 are in api/, a different shape of problem centred on PseudoNamespace, and belong in their own change.

Verified with black --check, flake8, the unit suite at 355 passing, and ascender --help plus ascender job_templates --help still rendering.

@ciq-it-service-account

ciq-it-service-account commented Sep 12, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

First substantive step on the typing work that ctrliq#42 only set up the measurement for. It takes the CLI from 48 type diagnostics to 2, and the package from 136 to 82, without annotating a single function signature.

The reason so much falls out of so little: `cli/custom.py` builds its actions out of mixins. `Launchable`, `HasStdout`, `AssociationMixin`, `RoleMixin` and `HasMonitor` all read `self.page`, `self.action` and `self.resource`, and none of them declares any of the three, because the `CustomAction` they get mixed into supplies them. That contract lived only in the reader's head, and it accounted for 46 of the diagnostics on its own.

Each mixin now states it:

```python
class Launchable:
    # Supplied by the CustomAction this is mixed into, which is why they are
    # annotations rather than assignments: they describe the contract without
    # creating class attributes that would shadow the real ones.
    page: 'api.pages.Page'
    action: str
    resource: str
```

Annotations without assignment create no class attribute, so there is no runtime change and nothing shadows what the subclasses set.

Three more, each a real finding rather than a checker being appeased:

**`CustomAction.action` and `.resource` were `@property` methods raising `NotImplementedError`.** Every subclass sets them as plain class attributes, `action = 'launch'`, so the properties were never reached and they conflicted with the mixin declarations above. They are annotations now, which is what the subclasses actually satisfy. `perform` keeps the property, since subclasses genuinely override it as a method.

**Two `add_arguments` overrides were narrower than what they replace.** `Launchable.add_arguments` takes `with_pk=True`; `BulkJobLaunch` and `AdhocCommandLaunch` declared it without that parameter and then passed `with_pk=False` to the parent by hand. They now carry the parameter with a `False` default and pass it through, so the behaviour is identical and the signature no longer contradicts the base.

**`NotificationAssociateMixin.targets` is not `dict[str, list[str]]`.** The three literals suggest it is, and then six `targets.update()` calls add entries like `['credentials', None]`. It is annotated as `dict[str, list[str | None]]`, which is what it has always held.

Two diagnostics remain in `cli/` and both are left on purpose:

- `cli/format.py:151`, `Cannot resolve imported module 'jq'`. It is the `formatting` extra, imported inside the function that needs it. Installing the extra in the type-check environment resolves it; the code is correct as it stands.
- `cli/utils.py:29`, `command.name` inside `CustomRegistryMeta.registry`. `type.__subclasses__()` is typed `list[type]`, which says nothing about the `name` every command declares. Fixing it means a `cast`, and contorting a metaclass to satisfy a checker is worse than the diagnostic.

The remaining 80 are in `api/`, a different shape of problem centred on `PseudoNamespace`, and belong in their own change.

Verified with `black --check`, `flake8`, the unit suite at 355 passing, and `ascender --help` plus `ascender job_templates --help` still rendering.
blaipr added a commit to blaipr/ascender-kit that referenced this pull request Sep 13, 2026
The `api/` counterpart to ctrliq#50, and the same shape of problem: mixins that read attributes the class they are mixed into supplies, with the contract written down nowhere. 131 type errors on `main` become 91, without annotating a single function signature.

`HasStatus` is the clearest case. It reads `self.status`, `self.get`, `self.related`, `self.walk`, `self.result_stdout`, `self.result_traceback`, `self.job_explanation`, `self.execution_environment`, `self.id` and `self.type`, and declares none of them, because the `Page` it is mixed into does. Fifteen diagnostics from one undocumented contract. `PageList` is the same with `json`, `connection`, `r`, `next`, `previous` and `get`.

Each now states what it needs:

```python
class HasStatus:
    # Supplied by the Page this is mixed into: the first three come from the
    # response body through Page.__getattr__, the rest are Page's own methods.
    # Annotations rather than assignments, so nothing is created at runtime and
    # nothing shadows what Page provides.
    status: str
    id: int
    type: str
    ...
```

Applied to `HasStatus` and `PageList`, then to the six mixins under `api/mixins/`: `has_copy`, `has_create`, `has_instance_groups`, `has_notifications`, `has_survey` and `has_variables`.

**One annotation was wrong and the checker said so, which is the point of having one.** `json: dict` on `HasVariables` and `HasCopy` produced two new errors, because the code does `self.json.variables` and `self.json.related`, and a plain `dict` has no such attributes. It is not a plain dict: `Page.__init__` stores a `PseudoNamespace`, which serves keys as attributes. Corrected to `json: PseudoNamespace`, and the two errors went with it.

Verified that this introduces nothing: the full diagnostic list before and after was diffed, and the set after is a strict subset. 40 errors retired, 0 added.

What is deliberately left, and why it is not in this change:

- **`Attribute 'id' is not defined on None`**, six of them, where a `.get()` that can return `None` is dereferenced immediately. These are real latent `AttributeError`s rather than missing declarations, and each needs a decision about what the absent case should do. That is a bug-fixing change, not a typing one.
- **`Cannot resolve imported module 'jq'` and `'simplejson'`**, both optional imports inside the functions that need them. Installing the extras in the type-check environment resolves both; the code is correct.
- **`HTTPBasicAuth.__call__` expects a `PreparedRequest`** in `pages/base.py`, where a `namedtuple` with a `headers` attribute is passed instead. That works because the callable only touches `headers`, but it is a genuine abuse of the interface and wants its own look.

Verified with `black --check`, `flake8`, the unit suite at 355 passing, `import ascenderkit`, and `ascender --help`.
@blaipr
blaipr force-pushed the feat/type-the-cli-base-classes branch from dc5b9b2 to 4682675 Compare September 13, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants