-
Notifications
You must be signed in to change notification settings - Fork 6
IHS-163 Add required change for IFC-1820 (display_label) #556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Deploying infrahub-sdk-python with
|
| Latest commit: |
54c6624
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e27c8bc6.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://gma-20250922-ifc1820.infrahub-sdk-python.pages.dev |
WalkthroughThe change adds an optional field display_label: str | None = None to BaseSchema in infrahub_sdk/schema/main.py. Test data in tests/unit/sdk/conftest.py updates the rfile_schema fixture, renaming the key from display_label to display_labels. This expands the BaseSchema’s attributes while adjusting the fixture input shape used in tests. No other logic, function signatures, or modules are modified in the described diff. Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## infrahub-develop #556 +/- ##
====================================================
- Coverage 75.97% 75.73% -0.25%
====================================================
Files 100 100
Lines 9345 8946 -399
Branches 1835 1757 -78
====================================================
- Hits 7100 6775 -325
+ Misses 1754 1689 -65
+ Partials 491 482 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
infrahub_sdk/schema/main.py (1)
270-271: New display_label field looks fine; clarify precedence vs display_labels.With both display_label (str|None) and display_labels (list[str]|None), consumers may be unsure which to use when both are set. Please confirm intended precedence and consider exposing a normalized accessor.
Suggested helper (non-breaking):
# Inside BaseSchema @property def effective_display_labels(self) -> list[str] | None: if self.display_labels: return self.display_labels if self.display_label: return [self.display_label] return NoneAlternatively, document that display_label is a backward‑compat input and display_labels is canonical.
Reference: infrahub_sdk/protocols_base.py Line 29 indicates a display_label on instances, which is distinct from schema but similar naming may confuse readers.
tests/unit/sdk/conftest.py (1)
823-823: Verify the path used in display_labels.You set display_labels to ["label__value"] for TransformJinja2, but the fixture’s attributes don’t define a label attribute. Is this intended (e.g., inherited attribute) or should it be ["name__value"] like other schemas?
If it’s a typo, apply:
- "display_labels": ["label__value"], + "display_labels": ["name__value"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrahub_sdk/schema/main.py(1 hunks)tests/unit/sdk/conftest.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/schema/main.pytests/unit/sdk/conftest.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Place and write unit tests under tests/unit/ (isolated component tests)
Files:
tests/unit/sdk/conftest.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the custom pytest plugin (infrahub_sdk.pytest_plugin) fixtures for clients, configuration, and Infrahub-specific testing
Files:
tests/unit/sdk/conftest.py
🧬 Code graph analysis (1)
infrahub_sdk/schema/main.py (1)
infrahub_sdk/protocols_base.py (1)
display_label(30-30)
Summary by CodeRabbit
New Features
Tests