IHS-183: Fix typing errors for protocols #749
Conversation
WalkthroughRuff version in .pre-commit-config.yaml was bumped from v0.11.9 to v0.14.10. Several type-ignore comments were removed (infrahub_sdk/ctl/branch.py, infrahub_sdk/testing/repository.py). CoreNodeBase, CoreNode, and CoreNodeSync were changed from runtime-checkable Protocols to concrete base classes with methods that raise NotImplementedError. RelatedNodeBase expanded _peer typing and adjusted duck-typed assignment logic. infrahub_sdk/schema/init.py now imports CoreNodeBase and uses a CoreNodeBase subclass check for schema-kind detection, with an updated error message. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
infrahub_sdk/node/related_node.py (1)
37-47: isinstance check will fail silently for InfrahubNode/InfrahubNodeSync instances.Line 37 type annotation allows
InfrahubNode | InfrahubNodeSync | CoreNodeBase, but line 46'sisinstance(data, CoreNodeBase)check will never match InfrahubNode or InfrahubNodeSync because neither inherits from CoreNodeBase. This means the peer assignment is skipped when data is an InfrahubNode or InfrahubNodeSync instance, contradicting the type annotation and creating unexpected behavior. Consider usingisinstance(data, (InfrahubNode, InfrahubNodeSync, CoreNodeBase))or having InfrahubNodeBase inherit from CoreNodeBase to align runtime type checking with the declared types.
Deploying infrahub-sdk-python with
|
| Latest commit: |
152d48c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://724391cd.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://sb-20260119-fix-typing-for-p.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #749 +/- ##
===========================================
+ Coverage 80.30% 80.35% +0.05%
===========================================
Files 115 115
Lines 9874 9872 -2
Branches 1518 1512 -6
===========================================
+ Hits 7929 7933 +4
Misses 1417 1417
+ Partials 528 522 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
infrahub_sdk/node/related_node.py
Outdated
| self._properties = self._properties_flag + self._properties_object | ||
|
|
||
| self._peer = None | ||
| self._peer: InfrahubNode | InfrahubNodeSync | InfrahubNodeBase | CoreNodeBase | None = None |
There was a problem hiding this comment.
could this be InfrahubNodeBase | CoreNodeBase | None? looks like the other two InfrahubNode classes inherit from InfrahubNodeBase
infrahub_sdk/schema/__init__.py
Outdated
| return schema | ||
|
|
||
| if hasattr(schema, "_is_runtime_protocol") and getattr(schema, "_is_runtime_protocol", None): | ||
| if isinstance(schema, type) and issubclass(schema, (CoreNode, CoreNodeSync)): |
There was a problem hiding this comment.
could this be issubclass(schema, (CoreNodeBase))?
|
|
||
| @runtime_checkable | ||
| class CoreNodeBase(Protocol): | ||
| class CoreNodeBase: |
There was a problem hiding this comment.
would be worth updating the PR description to explain why these are no longer Protocols
There was a problem hiding this comment.
Replace protocols with concrete classes to address type-abstract mypy errors
| # Check for InfrahubNodeBase instances using duck-typing (_schema attribute) | ||
| # to avoid circular imports, or CoreNodeBase instances | ||
| if isinstance(data, CoreNodeBase) or hasattr(data, "_schema"): | ||
| self._peer = cast("InfrahubNodeBase | CoreNodeBase", data) |
There was a problem hiding this comment.
maybe it is the best solution right now, but it smells
is the circular import error resolvable? I suspect it is probably pretty tangled, but maybe not
infrahub_sdk/protocols_base.py
Outdated
|
|
||
| def get_kind(self) -> str: ... | ||
| def get_kind(self) -> str: | ||
| return "" |
There was a problem hiding this comment.
These methods should not be called so as such I think we can just have a raise NotImplementedError instead of having these placeholder returns.
|
One thing that nags me about this is that we still have all of the nodes within a file called I don't think that it's worth renaming or moving around the files to correct this confusion. Just wanted to see if you guys had any thoughts around it? (@solababs, @ajtmccarty) |
|
maybe they could move to |
Fixes #697
Replace protocols with concrete classes to address
type-abstractmypy errorsSummary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.