Skip to content

fix: clear the remaining type diagnostics and enforce the check - #71

Merged
cigamit merged 1 commit into
ctrliq:mainfrom
blaipr:fix/clear-the-remaining-type-diagnostics
Sep 13, 2026
Merged

cigamit merged 1 commit into
ctrliq:mainfrom
blaipr:fix/clear-the-remaining-type-diagnostics

Conversation

@blaipr

@blaipr blaipr commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

The type check job was advisory while the diagnostics already in the tree were worked through. The last 23
are gone, so it now fails the merge like the other checks rather than reporting into a job nobody has to read.

What the diagnostics were, and what each one asked for

  • Overrides that narrowed their base signature. Token_Auth.__call__ renamed its parameter,
    Notification.wait_until_completed dropped **kwargs, and WorkflowJobRelaunch.add_arguments dropped
    with_pk, which the two other Launchable overrides already carry through.
  • Contracts a mixin relies on but does not declare. exceptions.Common.msg is the decoded response body
    rather than the empty string its default suggests, the registry metaclass reads a name off classes
    __subclasses__() describes as itself, and PageList reaches a Page constructor through self.__class__.
  • A property nothing ever reaches. CustomCommand.name raised NotImplementedError, but the registry
    reads it off the class, where subclasses set it as a plain attribute. It is now declared the way
    CustomAction already declares action and resource.
  • The request kwargs in Connection.request, whose inferred type does not admit the headers added to it
    further down.
  • The lazily built parser in sphinx.py, bound from the module __getattr__ that PEP 562 routes the
    first lookup to. Annotating it leaves the module namespace empty, so that lookup still arrives there, and
    gives the assignment a declaration to point at.
  • Optional imports. docutils and sphinxcontrib.autoprogram are the docs extra, which the job now
    installs. simplejson, jq and IPython are none of them a dependency, so the rule is turned off for the
    three files holding them through [[tool.ty.overrides]].

Why an override rather than a ty: ignore on the line

The narrower suppression is the wrong one here. Whether those three resolve depends on the environment the
checker runs in, and once the import does resolve the suppression goes unused, which is itself a diagnostic
and exits non-zero. Making the job enforcing is what turns that into a real cost: CI would go red over a
comment, on a change that never touched this code. jq is the standing case of the two, since it ships as a
compiled extension with no stubs and so fails to resolve whether or not it is installed.

Testing

Every job run locally on the interpreters CI uses, not just on the one to hand:

Job Result
Lint ruff format --check and ruff check clean
Test (3.11.16) 363 passed, 1 skipped
Test (3.14.7) 363 passed, 1 skipped
Type check (3.14.7) All checks passed!
Docs sphinx-build -W --keep-going succeeded
Build python -m build and twine check PASSED, wheel installs, ascender --help exits 0

The single skip is tests/unit/cli/test_sphinx.py, which wants the docs extra the Test job does not install.

The type check was also run with simplejson, IPython and jq installed and then absent, and comes back
clean either way, which is the point of the override.

@ciq-it-service-account

ciq-it-service-account commented Sep 13, 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 type check job was advisory while the diagnostics already in the tree were
worked through. The last 23 are gone, so it now fails the merge like the other
checks rather than reporting into a job nobody has to read.

What the diagnostics were, and what each one asked for:

- Overrides that narrowed their base signature. Token_Auth.__call__ renamed its
  parameter, Notification.wait_until_completed dropped **kwargs, and
  WorkflowJobRelaunch.add_arguments dropped with_pk, which the two other
  Launchable overrides already carry through.
- Contracts a mixin relies on but does not declare. exceptions.Common.msg is the
  decoded response body rather than the empty string its default suggests, the
  registry metaclass reads a name off classes __subclasses__() describes as
  itself, and PageList reaches a Page constructor through self.__class__.
- CustomCommand.name raised from a property that nothing ever reaches, since the
  registry reads it off the class where subclasses set it as an attribute. It is
  declared the way CustomAction already declares action and resource.
- The request kwargs in Connection.request, whose inferred type does not admit
  the headers added to it further down.
- sphinx.py binds its parser global from the module __getattr__ that PEP 562
  routes the first lookup to. Annotating it leaves the namespace empty, so the
  lookup still arrives there, and gives the assignment a declaration.
- Optional imports. docutils and sphinxcontrib.autoprogram are the docs extra,
  which the job now installs. simplejson, jq and IPython are none of them a
  dependency, so the rule is turned off for the three files holding them through
  [[tool.ty.overrides]]. A `ty: ignore` on the line would have been narrower and
  wrong: it goes unused, and an unused suppression is itself a diagnostic, the
  moment anything pulls one of the three into the environment. Turning the job
  enforcing is what makes that worth avoiding, since it would fail the merge
  over a comment rather than over the code.
@blaipr
blaipr force-pushed the fix/clear-the-remaining-type-diagnostics branch from 591c515 to 0e0134b Compare September 13, 2026 22:50
@cigamit cigamit self-assigned this Sep 13, 2026
@cigamit cigamit added the bug Something isn't working label Sep 13, 2026
@cigamit
cigamit merged commit 4354821 into ctrliq:main Sep 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

3 participants