Skip to content

Conversation

@solababs
Copy link
Contributor

@solababs solababs commented Jan 22, 2026

Fixes #697

Replace protocols with concrete classes to address type-abstract mypy errors

Summary by CodeRabbit

  • Refactor

    • Enhanced flexibility for related node handling to support duck-typing patterns.
    • Refactored internal class hierarchy for improved type safety and explicit error handling.
    • Updated schema validation logic for more robust type checking.
  • Chores

    • Updated Ruff development tool to latest version.
    • Removed unnecessary type-checking suppressions throughout codebase.
  • Tests

    • Added test coverage for schema name resolution.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 22, 2026

Walkthrough

This pull request converts Protocol-based type definitions to concrete base classes and updates related type handling throughout the SDK. Changes include removing @runtime_checkable decorators from CoreNodeBase, CoreNode, and CoreNodeSync in protocols_base.py, converting them to concrete classes with NotImplementedError implementations. Additional updates address protocol handling in schema validation, related node type checking, and schema name resolution logic. Dependencies on runtime protocol attributes were replaced with subclass checks and explicit type casting. Version updates and type-ignore directives were also removed from several files.

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.53% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: fixing typing errors related to protocols, which aligns with the primary objective of converting protocol types to concrete classes.
Linked Issues check ✅ Passed All changes directly address issue #697 by converting protocols to concrete classes (CoreNodeBase, CoreNode, CoreNodeSync) and updating type checks/imports, eliminating mypy 'type-abstract' errors when using generated kinds.
Out of Scope Changes check ✅ Passed All changes are directly related to converting protocols to concrete classes and removing type-ignore directives. The Ruff version update in .pre-commit-config.yaml is a minor tooling maintenance update tangential to the main fix.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link

Deploying infrahub-sdk-python with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6c5bd98
Status: ✅  Deploy successful!
Preview URL: https://5101b420.infrahub-sdk-python.pages.dev
Branch Preview URL: https://re-sb-20260119-fix-typing-fo.infrahub-sdk-python.pages.dev

View logs

@codecov
Copy link

codecov bot commented Jan 22, 2026

Codecov Report

❌ Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/store.py 70.00% 1 Missing and 2 partials ⚠️
infrahub_sdk/schema/__init__.py 33.33% 1 Missing and 1 partial ⚠️
@@             Coverage Diff             @@
##           develop     #763      +/-   ##
===========================================
+ Coverage    80.32%   80.38%   +0.05%     
===========================================
  Files          115      115              
  Lines         9888     9894       +6     
  Branches      1520     1517       -3     
===========================================
+ Hits          7943     7953      +10     
- Misses        1417     1418       +1     
+ Partials       528      523       -5     
Flag Coverage Δ
integration-tests 41.34% <11.53%> (-0.04%) ⬇️
python-3.10 51.41% <30.76%> (+<0.01%) ⬆️
python-3.11 51.43% <30.76%> (+0.02%) ⬆️
python-3.12 51.41% <30.76%> (+<0.01%) ⬆️
python-3.13 51.41% <30.76%> (+<0.01%) ⬆️
python-3.14 53.05% <30.76%> (-0.02%) ⬇️
python-filler-3.12 24.04% <50.00%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/ctl/branch.py 81.13% <ø> (ø)
infrahub_sdk/node/related_node.py 92.44% <100.00%> (ø)
infrahub_sdk/protocols_base.py 78.22% <100.00%> (+4.21%) ⬆️
infrahub_sdk/testing/repository.py 77.77% <ø> (ø)
infrahub_sdk/schema/__init__.py 69.42% <33.33%> (+0.08%) ⬆️
infrahub_sdk/store.py 77.47% <70.00%> (-0.12%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants