chore(ty): migrate from mypy to ty#126
Merged
Merged
Conversation
Pin ty==0.0.32 (matches ../infrahub) and configure environment.python-version to 3.10 — the project's floor — so checks reflect the broadest supported API. No suppressions and no CI integration yet; subsequent commits add the lint invoke task, capture the baseline, and drive per-module fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`uv run invoke lint` now runs ruff -> pylint -> yaml -> ty. The aggregate currently exits non-zero because ty reports the existing 302 diagnostics; subsequent commits suppress the baseline (T4) and then fix per module (T6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Baseline output from `uv run ty check .` plus rule-name and per-module diagnostic distributions. Used to drive [tool.ty.overrides] suppressions in the next commit; removed at the end of the migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Exclude examples/** from ty (generated sync_models.py inherits a dynamically resolved _ModelBaseClass that produces 144 unsupported-base warnings — not worth typing). Add [[tool.ty.overrides]] blocks for each remaining module covering exactly the rules that fire there today. Subsequent commits will remove these blocks one module at a time as real fixes land. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AGENTS.md (mirrored to CLAUDE.md via symlink) now references `uv run invoke linter.lint-ty` in the Quickstart and Required Development Workflow, and the Policy / Known Issues / Approval Checklist sections cite [[tool.ty.overrides]] and `# ty: ignore[<rule>]` instead of the mypy equivalents. Final mypy removal lands in a later commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extracted shared logic in _PrintCallVisitor into a private _visit_function helper accepting FunctionDef | AsyncFunctionDef so both visitor entrypoints type-check cleanly. Removed the tests/** override block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Guard optional `item.fields` with `or []` in has_field() so the generator module type-checks cleanly without the not-iterable override. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrap render_template's str literals in Path() at the call site so the generator signature stays Path-typed. Replace try/except TypeError around Path(config_file) with an explicit None guard so ty can narrow the type. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Annotate `self.flags` as `DiffSyncFlags` so subsequent `|=` against `str | DiffSyncFlags` values is well-typed, guard the optional `config.diffsync_flags` iteration, and resolve string items into enum members inline. Keep two targeted `# ty: ignore[invalid-attribute-access]` on `Adapter.top_level` assignments since diffsync declares it as a ClassVar but supports per-instance overrides at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wrapped module args to Collection.add_collection() with Collection.from_module() for sharper type resolution (invoke accepts modules at runtime, but its stubs require Collection). Fixed stale docs.generate_doc reference (renamed to docs.generate). Guarded optional Result from context.run() in docs.docusaurus. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-typed `print_error_and_abort` as `NoReturn` (it always raises `typer.Abort`), letting ty narrow `sync_instance` from `SyncInstance | None` to `SyncInstance` after the post-`get_instance` guards. Cast the schema mapping returned by `InfrahubClientSync.schema.all()` (API schema variants) to the `NodeSchema | GenericSchema` mapping expected by utils helpers. Removed the cli.py override block from pyproject.toml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uppression Annotate validator_kwargs and FILTERS_OPERATIONS to collapse Pydantic v1/v2 overload unions, declare DiffSyncMixin.top_level as a ClassVar, narrow the optional _add_custom_filters hook via getattr, raise on missing mapping in get_resource_name, and look up Pydantic fields via model_fields (v2) with a __fields__ fallback (v1). Single # ty: ignore[no-matching-overload] kept on the version-gated @validator_decorator call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Removed the override block for `infrahub_sync/adapters/**` from
`pyproject.toml` (last remaining ty suppression).
- Real fixes (no ignore needed):
- `SchemaMappingModel.fields` is now non-Optional with
`default_factory=list`; eliminates 3 `not-iterable` errors.
- `DiffSyncModelMixin.local_id` declared on the mixin so static
checkers see it on subclasses (covers most `local_id` references).
- Every `model_loader(..., model: <Model>)` and `*_obj_to_diffsync`
helper now correctly takes `type[<Model>]` instead of an instance.
- `DiffSyncModel.create(...)` overrides now take `dict[Any, Any]` (the
diffsync base shape) instead of `Mapping[Any, Any]`, restoring LSP.
- Direct import `from typing_extensions import Self` (project targets
3.10 where `typing.Self` doesn't exist; ty couldn't follow the
try/except shim).
- `slurpitsync.slurpit_obj_to_diffsync` return type is now `dict | None`
(matched the actual code path that returns `None`).
- `genericrestapi._extract_objects_from_response` introduces a typed
`result` variable to satisfy the declared return type.
- `prometheus._ensure_samples`/`model_loader` use locally-bound
variables so `_lookup` / `_samples_by_metric` narrow correctly.
- `infrahub.py`: `resolve_peer_node` now guards `client is None` on the
fallback path; `diffsync_to_infrahub`/`infrahub_node_to_diffsync`
skip when `schemas.get(peer)` returns None; `InfrahubModel.create`
and `.update` assert `isinstance(adapter, InfrahubAdapter)` so all
runtime-only attributes (`client`, `source_node`, `owner_node`,
`schema`) narrow without ignores; `node_schema` parameter retyped
to `NodeSchemaAPI` (was `NodeSchema`, which is missing
`relationship_names`/`relationships`).
- `client.all(..., include=list(model._attributes), ...)` — coerce
the `Tuple[str, ...]` ClassVar to `list[str]` for the overload.
- `# ty: ignore` annotations (each with a stated reason):
- `unresolved-import` (5): `pynetbox`, `pynautobot`, `slurpit`,
`ipfabric`, `prometheus_client.parser` — all optional extras.
- `unresolved-attribute` (9): `self.type.title()` across 7 adapters
(base `Adapter.type` is `Optional[str]`; override is non-None at
runtime), `adapter.config`/`adapter.store`/`adapter.client` calls
in `utils.build_mapping` and `peeringmanager.update` (runtime attrs
attached by concrete sync adapters), and `node.local_id` on
store-returned `DiffSyncModel` (typing limitation:
`DiffSync.store.get_all()` returns the base class).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codebase is clean under ty across 3.10/3.11/3.12/3.13 after T6's per-module fix loop. Promote `uv run ty check .` from local-only to a required CI step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop [tool.mypy], the mypy>=1.15 dev dep, the duplicate types-pyyaml stub (keeping canonical types-PyYAML>=6.0.12), and the 13 remaining # type: ignore[...] directives (deleted as obsolete or converted to # ty: ignore[<rule>] where ty still flags the line). Migration scratch file docs/superpowers/notes/ty-baseline.txt is also removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
BeArchiTek
added
group/ci
Deploying infrahub-sync with
|
| Latest commit: |
9c571f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d56a2f2c.infrahub-sync.pages.dev |
| Branch Preview URL: | https://chore-migrate-to-ty.infrahub-sync.pages.dev |
Contributor
WalkthroughThis PR replaces mypy with ty (CI, invoke task, pyproject config, docs) and adds a new CI step and invoke task to run 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 59: The policy references a `[tool.ty.overrides]` block in
`pyproject.toml` that does not exist; update AGENTS.md to either add a matching
`tool.ty.overrides` section to the repository pyproject.toml or change the
wording to remove/clarify that reference (e.g., instruct maintainers to use
existing type-check override mechanisms or use targeted `# ty: ignore[...]`),
and ensure the message and symbol `[tool.ty.overrides]` in AGENTS.md/lines ~59
and ~208 are consistent with the actual `pyproject.toml`.
In `@infrahub_sync/adapters/genericrestapi.py`:
- Around line 205-215: The returned list may contain non-dict entries (from the
list branch or dict.values()), which breaks the declared list[dict[str, Any]]
contract and downstream code like obj_to_diffsync; ensure every element is a
dict: after building result from objs (dict -> list(objs.values()), list ->
objs, else -> [objs] or []), transform/filter result so each item is either kept
if isinstance(item, dict), skipped if item is None/empty, or wrapped into a
small dict (e.g., {'_value': item}) for non-dict scalars; update the code that
constructs result (use objs, result) to perform this normalization and preserve
the list[dict[str, Any]] type before returning.
- Line 163: Guard against None before calling .title() on adapter type:
normalize both sides once (e.g. using safe conversion like
(self.config.source.name or "") and (self.type or "")), trim and lower (or
title) them, then compare the normalized strings instead of calling .title()
directly; update the comparison that currently uses
self.config.source.name.title() == self.type.title() to use the normalized
values so AttributeError won't occur when adapter_type is None.
In `@infrahub_sync/adapters/infrahub.py`:
- Around line 455-461: Replace the runtime assertions with explicit exceptions:
instead of assert isinstance(adapter, InfrahubAdapter) raise a TypeError with a
clear message referencing adapter and InfrahubAdapter, and instead of assert
isinstance(node_schema, NodeSchemaAPI) raise a TypeError referencing node_schema
and NodeSchemaAPI; apply the same replacement for the similar checks around
lines 478-481 (same pattern). Ensure the error messages include contextual info
(e.g., cls.__name__ or adapter) so failures are informative at runtime.
In `@infrahub_sync/adapters/slurpitsync.py`:
- Around line 169-170: Add concise docstrings to the public methods
model_loader, slurpit_obj_to_diffsync, create, and update: each docstring should
be a one- to three-line summary of the method purpose followed by brief param
descriptions for visible arguments (e.g., model_name: str, model:
type[SlurpitsyncModel], slurpit object, diffsync instance), the return value (or
None) and any raised exceptions; place the docstrings immediately below each def
line for model_loader, slurpit_obj_to_diffsync, create, and update so they
satisfy the "Public functions and classes require concise docstrings" guideline.
- Line 8: Update the two "ty: ignore" comments to include a TODO note: change
the import line for slurpit (import slurpit # ty: ignore[unresolved-import]) to
include a short TODO explaining why it's ignored and when to remove it, and
replace the second ty: ignore (the one used to silence a type mismatch at the
conversion/typing site) with either a narrowed explicit type annotation or a
TODOed ty: ignore that documents why the check is bypassed and how to fix it
later; additionally add concise docstrings to the public functions model_loader,
slurpit_obj_to_diffsync, and create describing their purpose, parameters, and
return values so each public method has a proper docstring per guidelines.
In `@infrahub_sync/utils.py`:
- Around line 154-158: The existing branch that constructs config_file_path
incorrectly treats an empty-string directory as falsy and leaves
config_file_path None; update the conditional so empty-string directories are
accepted (e.g., replace the `elif directory:` guard with a check for not-None
like `elif directory is not None:` or simply use `else:`) so that when
`config_file` is not absolute and `directory == ""` you still return
Path(directory, config_file); refer to the config_file_path variable and the two
Path(...) constructors in this block to locate and modify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea7770ff-628c-49a9-a8e1-1ffca2c95b0a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.github/workflows/workflow-linter.ymlAGENTS.mdinfrahub_sync/__init__.pyinfrahub_sync/adapters/aci.pyinfrahub_sync/adapters/genericrestapi.pyinfrahub_sync/adapters/infrahub.pyinfrahub_sync/adapters/ipfabricsync.pyinfrahub_sync/adapters/nautobot.pyinfrahub_sync/adapters/netbox.pyinfrahub_sync/adapters/peeringmanager.pyinfrahub_sync/adapters/prometheus.pyinfrahub_sync/adapters/slurpitsync.pyinfrahub_sync/adapters/utils.pyinfrahub_sync/cli.pyinfrahub_sync/generator/__init__.pyinfrahub_sync/potenda/__init__.pyinfrahub_sync/utils.pypyproject.tomltasks/__init__.pytasks/docs.pytasks/linter.pytests/__init__.pytests/test_logging.py
…ter mismatch) `build_mapping(adapter: Adapter, ...)` in adapters/utils.py accessed `adapter.config` and `adapter.store`. Neither attribute exists on `diffsync.Adapter`; both are added by our `DiffSyncMixin` (config) and `diffsync.Adapter`'s own `__init__` (store). The previous T6.8 comment described `config` as "attached at runtime by SyncAdapter subclasses", which conflated two unrelated types: `SyncAdapter` is a Pydantic config model holding YAML adapter settings, and nothing subclasses it. Declare `config: SyncConfig` and `store: BaseStore` on `DiffSyncMixin` so ty resolves the attribute chain on real adapter instances directly. Retype `build_mapping`'s parameter as `DiffSyncMixin` (every caller passes a `DiffSyncMixin + Adapter` subclass) and drop the now-redundant `# ty: ignore[unresolved-attribute]` and the misleading comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- AGENTS.md: Policy / Known Issues no longer reference `[tool.ty.overrides]`
blocks — there are none in pyproject.toml after T6.
- genericrestapi.py: guard `self.type.title()` (the `adapter_type` __init__
param is `str | None`); filter `_extract_objects_from_response` to dicts
so the declared `list[dict[str, Any]]` contract holds.
- infrahub.py: replace `assert isinstance(...)` runtime guards in
InfrahubModel.create/update with explicit `raise TypeError(...)` (project
policy: raise specific exceptions; asserts disappear under `python -O`).
- slurpitsync.py: add docstrings to model_loader, slurpit_obj_to_diffsync,
create, update.
- utils.py: empty-string `directory` now matches the pre-refactor behavior
(`Path("", config_file)` collapses to `Path(config_file)`) — `elif directory:`
was a regression introduced in T6.3.
Skipped: TODO labels on `# ty: ignore[unresolved-import]` for optional deps
(pynetbox, pynautobot, slurpit, ipfabric, prometheus_client.parser). Those
imports are intentionally optional per `[project.optional-dependencies]`;
there is no "remove when X" action to TODO. The existing reason comment
("optional dep, see pyproject extras") is the stable design rationale.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI caught the multi-line ternary on python-lint (3.13). `ruff format` collapses it; no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uns ty `uv run invoke lint` already runs ruff → pylint → yamllint → ty. Calling `uv run invoke linter.lint-ty` separately in the Quickstart and Required Development Workflow was redundant and misleading. Updated the Code Standards bullet to match, and added `linter.lint-ty` to the invoke task reference list (it was missing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dgarros
approved these changes
May 19, 2026
Comments removed: - The 3-line block comments on `DiffSyncMixin` annotations (top_level / config / store / local_id) — the annotations on a mixin class are self-explanatory. - The 8 `# typing.Self is Python 3.11+; ...` headers above each `from typing_extensions import Self` — the import line is the rationale. - The 7 `# self.type is overridden as a non-None ClassVar; ty sees the base Optional[str]` narrations preceding `# ty: ignore[unresolved-attribute]` — the rule code is enough. - The 3 `# get_value returns Any | None; ...` narrations preceding `or []`. - The verbose multi-line shim/decorator headers in __init__.py (kept one-liner). - The 2-line `# adapter is typed as the diffsync base for Liskov ...` and `# self.adapter is Adapter | None ...` preceding the explicit TypeError raises — the error messages already explain the contract. - The 2-line `# diffsync's Adapter.top_level is a ClassVar ...` in potenda. - `# Find the schema element matching the model name.` narrates WHAT. - `# diffsync.Adapter declares type: Optional[str]` narrates the obvious. - `# local_id is declared on generated subclasses` repeated under the existing mixin-level docstring. Net: -70 / +9 across 13 files; ty / ruff / tests all stay green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 124: The sentence about ty is inconsistent: replace the phrase "do not
increase the error count" with explicit zero-baseline wording so it requires `uv
run ty check .` to report 0 errors; update the line that mentions `uv run invoke
lint`/`ty` to read that contributors must ensure `uv run ty check .` returns
zero errors (or equivalent explicit zero-baseline phrasing) so the document
aligns with the repo policy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bc9a216-9544-4a22-aebd-e13811b3e99c
📒 Files selected for processing (14)
AGENTS.mdinfrahub_sync/__init__.pyinfrahub_sync/adapters/genericrestapi.pyinfrahub_sync/adapters/infrahub.pyinfrahub_sync/adapters/ipfabricsync.pyinfrahub_sync/adapters/nautobot.pyinfrahub_sync/adapters/netbox.pyinfrahub_sync/adapters/peeringmanager.pyinfrahub_sync/adapters/prometheus.pyinfrahub_sync/adapters/slurpitsync.pyinfrahub_sync/adapters/utils.pyinfrahub_sync/cli.pyinfrahub_sync/potenda/__init__.pyinfrahub_sync/utils.py
💤 Files with no reviewable changes (4)
- infrahub_sync/adapters/prometheus.py
- infrahub_sync/adapters/ipfabricsync.py
- infrahub_sync/adapters/nautobot.py
- infrahub_sync/adapters/netbox.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Chores
Documentation
Replaces mypy with ty (Astral's new type checker, pinned to
0.0.32to match../infrahub). ty is faster, catches more issues, and aligns this repo with the rest of the OpsMill Python stack.Carried out the migration as a careful per-module cleanup:
tydev dep + minimal[tool.ty]config; wirelinter.lint-tyintoinvoke lint.examples/**from ty (generatedsync_models.pycarries 144unsupported-basewarnings on its dynamic base class — not worth typing).[[tool.ty.overrides]]per module covering exactly the rules that fire there today, so ty exits 0 without any code changes yet.Linting: ty checkas a blocking step in.github/workflows/workflow-linter.yml.[tool.mypy], drop the dep, remove duplicatetypes-pyyaml, rewrite the 13 stale# type: ignoredirectives.Real bugs ty surfaced (vs. mypy)
Seven genuine bugs caught during the per-module fix loop:
_PrintCallVisitor.visit_AsyncFunctionDefwas typing its argument asFunctionDef(tests/test_logging.py).tasks/__init__.pyhad a stale reference todocs.generate_doc— the renamed function isdocs.generate.cli.print_error_and_abortwas annotated as returningtyper.Abortbut always raises; missingNoReturndefeated narrowing acrosssync_cmd/diff_cmd/generate(19 diagnostics collapsed into one fix).infrahub_sync.get_resource_namecould silently returnNonewhen a schema mapping had a name match but nomappingfield.adapters/infrahub.resolve_peer_nodecould crash onclient.get()whenclientwasNoneon the fallback path.adapters/infrahubdiffsync_to_infrahub/infrahub_node_to_diffsyncsilently usedNonepeer schemas.adapters/slurpitsync.slurpit_obj_to_diffsyncwas annotated-> dictbut returnedNonein the skip path.Net before/after
No more
[[tool.ty.overrides]]blocks; no more# type: ignoredirectives in the codebase.CI
New blocking step in
python-lint, running across the existing3.10 / 3.11 / 3.12 / 3.13matrix.Test plan
uv run invoke linter.lint-tyexits 0.uv run pytest -o "addopts=" tests/ -q→ 8 passed.uv run infrahub-sync list --directory examples/lists all 18 projects.uv run infrahub-sync --helpexits 0.🤖 Generated with Claude Code