Revert "IHS-183: Fix typing errors for protocols" #760
Conversation
This reverts commit 6c0bf35.
WalkthroughThis pull request refactors core node handling by converting 🚥 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 |
Deploying infrahub-sdk-python with
|
| Latest commit: |
8f70377
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4b23771d.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://revert-749-sb-20260119-fix-t.infrahub-sdk-python.pages.dev |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #760 +/- ##
===========================================
- Coverage 80.38% 80.32% -0.06%
===========================================
Files 115 115
Lines 9886 9888 +2
Branches 1514 1520 +6
===========================================
- Hits 7947 7943 -4
Misses 1417 1417
- Partials 522 528 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.pre-commit-config.yaml (1)
14-22: Downgrade Ruff to v0.11.9 needs justification.The version
v0.11.9(May 9, 2025) is a significant downgrade from v0.14.10 (December 18, 2025) and the latest v0.14.11 (January 8, 2026). This regression masks formatter improvements, new rules (e.g., ISC004 for implicit string concatenation), and bug fixes from the 0.14.x releases. Please confirm this downgrade is intentional and document the specific reason (e.g., incompatibility, false positives in newer versions, performance concerns).
🤖 Fix all issues with AI agents
In `@infrahub_sdk/schema/__init__.py`:
- Around line 210-217: The conditional unnecessarily uses both hasattr and
getattr to check the runtime protocol marker; replace the two-call pattern with
a single getattr(schema, "_is_runtime_protocol", False) check (keeping the rest
of the branch logic that inspects schema.save and schema.__name__ unchanged) so
the code reads: if getattr(schema, "_is_runtime_protocol", False): — this
removes the redundant hasattr while preserving behavior for schema, schema.save,
and the name-handling branches.
🧹 Nitpick comments (2)
infrahub_sdk/node/related_node.py (2)
37-50: Missing type annotation on_peerreduces type safety.The
_peerattribute lacks a type annotation, which means the type checker cannot infer its type in property accessors likeid,hfid,display_label, etc. This leads to thetype: ignore[return-value]comments needed in theget()methods (lines 249, 252, 255, 296, 299, 302).Consider adding a type annotation to preserve type safety:
♻️ Suggested type annotation
- self._peer = None + self._peer: CoreNodeBase | None = None
46-47: Unnecessary parentheses around single type inisinstancecheck.The parentheses around
CoreNodeBaseare not needed when checking against a single type.✨ Minor style fix
- if isinstance(data, (CoreNodeBase)): + if isinstance(data, CoreNodeBase):
| if hasattr(schema, "_is_runtime_protocol") and getattr(schema, "_is_runtime_protocol", None): | ||
| if inspect.iscoroutinefunction(schema.save): | ||
| return schema.__name__ | ||
| if schema.__name__[-4:] == "Sync": | ||
| return schema.__name__[:-4] | ||
| return schema.__name__ | ||
|
|
||
| raise ValueError("schema must be a CoreNode subclass or a string") | ||
| raise ValueError("schema must be a protocol or a string") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n infrahub_sdk/schema/__init__.py | head -250 | tail -60Repository: opsmill/infrahub-sdk-python
Length of output: 2672
🌐 Web query:
Python _is_runtime_protocol attribute typing runtime_checkable
💡 Result:
Short answer: decorating a Protocol with typing.runtime_checkable sets an internal flag (attribute) on the Protocol class (commonly _is_runtime_protocol = True) so the typing machinery will allow and perform structural isinstance()/issubclass() checks at runtime. The runtime checks only verify that required members exist (not their signatures or value types). Python 3.12 also changed the lookup to use inspect.getattr_static() and “freezes” the protocol members when the class is created. [1][2][3]
Sources:
- typing.runtime_checkable docs (behavior and notes about members/signatures, 3.8+ and 3.12 changes). [1]
- PEP 544 (describes _is_protocol and runtime_checkable intent). [2]
- typing/typing_extensions implementation showing the decorator sets cls._is_runtime_protocol = True. [3]
🏁 Script executed:
# Check if there are other uses of _is_runtime_protocol in the codebase
rg "_is_runtime_protocol" --type py -B 2 -A 2Repository: opsmill/infrahub-sdk-python
Length of output: 759
🏁 Script executed:
# Find where _get_schema_name is called and what types are passed to it
rg "_get_schema_name" --type py -B 3 -A 3Repository: opsmill/infrahub-sdk-python
Length of output: 2397
🏁 Script executed:
# Check imports and type definitions for SchemaType and SchemaTypeSync
rg "SchemaType\|SchemaTypeSync" --type py -B 1 -A 1 | head -100Repository: opsmill/infrahub-sdk-python
Length of output: 53
🏁 Script executed:
# Look for where `@runtime_checkable` is used
rg "@runtime_checkable\|runtime_checkable" --type py -B 2 -A 5Repository: opsmill/infrahub-sdk-python
Length of output: 53
🏁 Script executed:
# Search for SchemaType and SchemaTypeSync class definitions
rg "^class SchemaType|^SchemaType = " --type py -A 10Repository: opsmill/infrahub-sdk-python
Length of output: 607
🏁 Script executed:
# Check imports at top of schema/__init__.py
head -50 infrahub_sdk/schema/__init__.pyRepository: opsmill/infrahub-sdk-python
Length of output: 1221
🏁 Script executed:
# Look for Protocol definitions in the codebase
rg "class.*Protocol" --type py -B 2 -A 3Repository: opsmill/infrahub-sdk-python
Length of output: 3118
Simplify the redundant hasattr check.
The hasattr call is unnecessary since getattr(schema, "_is_runtime_protocol", None) already returns a falsy value if the attribute doesn't exist. Simplify to:
if getattr(schema, "_is_runtime_protocol", False):Note: While _is_runtime_protocol is an internal implementation detail, it is the documented mechanism (per PEP 544) for checking if a class is decorated with @runtime_checkable. This is the correct approach and has been stable since Python 3.8.
🤖 Prompt for AI Agents
In `@infrahub_sdk/schema/__init__.py` around lines 210 - 217, The conditional
unnecessarily uses both hasattr and getattr to check the runtime protocol
marker; replace the two-call pattern with a single getattr(schema,
"_is_runtime_protocol", False) check (keeping the rest of the branch logic that
inspects schema.save and schema.__name__ unchanged) so the code reads: if
getattr(schema, "_is_runtime_protocol", False): — this removes the redundant
hasattr while preserving behavior for schema, schema.save, and the name-handling
branches.
solababs
deleted the
revert-749-sb-20260119-fix-typing-for-protocols-ihs-183
branch
Reverts #749
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.