Skip to content

feat: declare the attribute contracts the api mixins rely on - #51

Open
blaipr wants to merge 1 commit into
ctrliq:mainfrom
blaipr:feat/type-the-api-pages
Open

feat: declare the attribute contracts the api mixins rely on#51
blaipr wants to merge 1 commit into
ctrliq:mainfrom
blaipr:feat/type-the-api-pages

Conversation

@blaipr

@blaipr blaipr commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

The api/ counterpart to #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:

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 AttributeErrors 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.

@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.

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 added a commit to blaipr/ascender-kit that referenced this pull request Sep 13, 2026
Six places call `.get()` twice on the same dictionary, once to test it and once to use it:

```python
if kwargs.get('project'):
    payload.update(project=kwargs.get('project').id, playbook=playbook)
```

That is correct. The guard does protect the access, and nothing mutates the dictionary in between. It reads poorly, it does the lookup twice, and a type checker cannot connect the two calls, so each one shows up as "Attribute `id` is not defined on `None`". Binding once fixes all three at no cost:

```python
project = kwargs.get('project')
if project:
    payload.update(project=project.id, playbook=playbook)
```

Applied in `job_templates.py`, `projects.py`, `workflow_job_templates.py` twice, and `page.py` twice.

**One in the same family is a real improvement rather than a tidy-up.** `logged_sleep` did this:

```python
try:
    frm = inspect.stack()[stack_depth]
    logger = logging.getLogger(inspect.getmodule(frm[0]).__name__)
except AttributeError:  # module is None (interactive shell)
    logger = log
```

The comment is right about why: `inspect.getmodule` returns `None` for a frame with no module, which is what an interactive shell gives. But catching `AttributeError` to detect that also swallows any other `AttributeError` raised inside the `try`, including one from `inspect.stack()` itself. The `None` is now tested for directly, so only the case the comment describes is handled and anything else surfaces.

**Correcting something I said in ctrliq#42 and ctrliq#51.** I described these as latent `AttributeError`s waiting to happen. They are not: every one is guarded. They are a readability and double-lookup issue that a checker happens to notice, and this change is worth making on those grounds, not on a correctness scare.

Seven diagnostics retired, 131 to 124. Diffed the full list before and after: strict subset, nothing introduced.

Verified with `black --check`, `flake8`, the unit suite at 355 passing, and `logged_sleep(0)` exercising the rewritten path.
@blaipr
blaipr force-pushed the feat/type-the-api-pages branch from 2281b66 to 5088ba1 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