Fix issue #713 and #712 #714
Conversation
WalkthroughStrips Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## stable #714 +/- ##
==========================================
+ Coverage 76.03% 76.84% +0.80%
==========================================
Files 113 113
Lines 9744 9768 +24
Branches 1491 1497 +6
==========================================
+ Hits 7409 7506 +97
+ Misses 1840 1757 -83
- Partials 495 505 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
Deploying infrahub-sdk-python with
|
| Latest commit: |
f96fb55
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eee8b025.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://bgi-fix-713.infrahub-sdk-python.pages.dev |
github-actions
bot
added
the
type/documentation
![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
🧹 Nitpick comments (1)
tests/unit/ctl/test_graphql_utils.py (1)
1-242: Comprehensive test coverage with excellent structure.The tests thoroughly validate the typename-stripping functionality across various GraphQL query structures, including edge cases like inline fragments, aliases, arguments, and directives. All tests follow best practices with clear naming and proper type hints.
However, consider relocating this test file to better align with the module structure:
Current location:
tests/unit/ctl/test_graphql_utils.py
Module being tested:infrahub_sdk.graphql.utils
Recommended location:tests/unit/graphql/test_utils.pyTest files should mirror the source module structure rather than where the functionality is used. This improves discoverability and maintains consistency in the test organization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/docs/python-sdk/guides/python-typing.mdxinfrahub_sdk/ctl/graphql.pyinfrahub_sdk/graphql/utils.pytests/unit/ctl/test_graphql_utils.py
🧰 Additional context used
📓 Path-based instructions (8)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
infrahub_sdk/graphql/utils.pytests/unit/ctl/test_graphql_utils.pyinfrahub_sdk/ctl/graphql.py
infrahub_sdk/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Follow async/sync dual pattern for new features in the Python SDK
Files:
infrahub_sdk/graphql/utils.pyinfrahub_sdk/ctl/graphql.py
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{md,mdx}: Usetextlanguage for directory structure code blocks in markdown documentation
Add blank lines before and after lists in markdown documentation
Always specify language in fenced code blocks in markdown documentation
Files:
docs/docs/python-sdk/guides/python-typing.mdx
docs/docs/**/*.mdx
📄 CodeRabbit inference engine (docs/AGENTS.md)
docs/docs/**/*.mdx: Create MDX files in appropriate directory within docs structure (guides, topics, or reference)
Add frontmatter withtitlefield to MDX documentation files
Use callouts (:::warning, :::note, etc.) for important notes and information in documentation
Files:
docs/docs/python-sdk/guides/python-typing.mdx
docs/docs/python-sdk/{guides,topics}/**/*.mdx
📄 CodeRabbit inference engine (docs/AGENTS.md)
docs/docs/python-sdk/{guides,topics}/**/*.mdx: Use Tabs component from '@theme/Tabs' for async/sync examples in documentation
Include both async/sync examples using Tabs in documentation
Files:
docs/docs/python-sdk/guides/python-typing.mdx
tests/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
tests/**/*.py: Usehttpx_mockfixture for HTTP mocking in tests instead of making real HTTP requests
Do not add@pytest.mark.asynciodecorator to async test functions (async auto-mode is globally enabled)
Files:
tests/unit/ctl/test_graphql_utils.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
Unit tests must be fast, mocked, and have no external dependencies
Files:
tests/unit/ctl/test_graphql_utils.py
infrahub_sdk/ctl/**/*.py
📄 CodeRabbit inference engine (infrahub_sdk/ctl/AGENTS.md)
infrahub_sdk/ctl/**/*.py: Use@catch_exception(console=console)decorator on all async CLI commands
IncludeCONFIG_PARAMin all CLI command function parameters, even if unused
Useinitialize_client()orinitialize_client_sync()from ctl.client for client creation in CLI commands
Use Rich library for output formatting in CLI commands (tables, panels, console.print) instead of plain print() statements
Do not instantiate InfrahubClient directly; always use initialize_client() or initialize_client_sync() helper functions
Do not use plain print() statements in CLI commands; use Rich console.print() instead
Files:
infrahub_sdk/ctl/graphql.py
🧠 Learnings (2)
📚 Learning: 2025-12-10T17:13:14.678Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: docs/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:14.678Z
Learning: Applies to docs/python-sdk/reference/config.mdx : Do not edit `docs/python-sdk/reference/config.mdx` directly; regenerate with `uv run invoke generate-sdk` instead
Applied to files:
docs/docs/python-sdk/guides/python-typing.mdx
📚 Learning: 2025-12-10T17:13:21.977Z
Learnt from: CR
Repo: opsmill/infrahub-sdk-python PR: 0
File: infrahub_sdk/ctl/AGENTS.md:0-0
Timestamp: 2025-12-10T17:13:21.977Z
Learning: Applies to infrahub_sdk/ctl/**/*.py : Use `initialize_client()` or `initialize_client_sync()` from ctl.client for client creation in CLI commands
Applied to files:
infrahub_sdk/ctl/graphql.py
🧬 Code graph analysis (2)
tests/unit/ctl/test_graphql_utils.py (1)
infrahub_sdk/graphql/utils.py (3)
strip_typename_from_fragment(72-84)strip_typename_from_operation(57-69)strip_typename_from_selection_set(14-54)
infrahub_sdk/ctl/graphql.py (1)
infrahub_sdk/graphql/utils.py (4)
insert_fragments_inline(96-114)remove_fragment_import(117-123)strip_typename_from_fragment(72-84)strip_typename_from_operation(57-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (10)
infrahub_sdk/ctl/graphql.py (4)
25-30: LGTM! Clean imports for typename stripping utilities.The imports are well-organized and bring in the necessary utilities for stripping
__typenamefields from GraphQL operations and fragments.
160-164: Excellent explanation and implementation of typename stripping.The comment clearly explains why
__typenamemust be removed (ariadne-codegen validation issue), and the implementation correctly applies the stripping functions to both queries and fragments before code generation.
168-168: LGTM! Consistent use of stripped operations and fragments.The stripped versions are correctly passed to the package generator and used in the iteration loop, maintaining consistency throughout the code generation flow.
Also applies to: 180-180
171-171: Good defensive programming with the fallback value.The fallback to
"graphql_client"whendirectory.nameis empty/falsy prevents potential issues with unnamed directories.docs/docs/python-sdk/guides/python-typing.mdx (2)
114-114: LGTM! Documentation accurately reflects the updated workflow.The changes clearly document the directory-based approach for generating Pydantic models and include the schema export step, providing better guidance to users.
Also applies to: 123-130
134-170: Excellent structured workflow with helpful warnings.The numbered steps provide clear guidance, and the warning callout appropriately alerts users to the potential file-naming conflicts when queries share names. The example code is complete and practical.
infrahub_sdk/graphql/utils.py (4)
3-11: LGTM! Necessary imports for GraphQL AST manipulation.All required GraphQL AST node types are imported for the typename-stripping functionality.
14-54: Excellent implementation of recursive typename stripping.The function correctly:
- Handles the None case early
- Skips
__typenamefields while preserving all other fields- Recursively processes nested selection sets
- Preserves all node attributes (aliases, arguments, directives)
- Handles inline fragments and fragment spreads appropriately
- Returns a new immutable SelectionSetNode
The docstring clearly explains the motivation (ariadne-codegen's inability to look up
__typenamein the schema).
57-69: LGTM! Clean operation wrapper for typename stripping.The function properly creates a new
OperationDefinitionNodewith a stripped selection set while preserving all other operation attributes.
72-84: LGTM! Clean fragment wrapper for typename stripping.The function properly creates a new
FragmentDefinitionNodewith a stripped selection set while preserving all fragment attributes.
ajtmccarty
left a comment
ajtmccarty
left a comment
There was a problem hiding this comment.
not sure exactly how much testing we have for infrahubctl commands, but it would be good to have a test for this that actually calls generate_return_types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/ctl/test_graphql_app.py (2)
68-103: Invalid-query test is good; message match could be made more future-proofThe invalid query fixture and use of the real schema give good coverage of validation failures. The assertion
match="Cannot query field"is reasonable but couples the test to the exact GraphQL error wording; if the underlying GraphQL library ever changes this phrasing, the test may fail even though behavior is still correct. You could consider matching a more generic pattern (e.g., just checking that aValueErroris raised, or matching on the invalid field name) to reduce brittleness, but it's optional.
105-266: CLI tests are comprehensive; a couple of tiny optional assertion improvementsThe
generate-return-typestests nicely cover:
- single-file vs directory input
- missing schema
- invalid queries (non-fatal)
__typenamehandling- default CWD behavior
- empty directories
- multiple queries in one directory.
Two small, optional tightenings you might consider:
- In
test_generate_return_types_no_gql_files, the comment mentions “no output”; you could assertresult.stdout == ""(or.strip() == "") to capture that contract explicitly.- In
test_generate_return_types_invalid_query, you already assert"Error" in clean_output; optionally asserting that the invalid filename or query name appears too would make it clearer that the error is tied to the right input.These are niceties only; the current tests are already quite strong.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/fixtures/unit/test_infrahubctl/graphql/invalid_query.gqltests/fixtures/unit/test_infrahubctl/graphql/query_with_typename.gqltests/fixtures/unit/test_infrahubctl/graphql/test_schema.graphqltests/fixtures/unit/test_infrahubctl/graphql/valid_query.gqltests/unit/ctl/test_graphql_app.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use type hints on all function signatures
Never mix async/sync inappropriately
Never bypass type checking without justification
Files:
tests/unit/ctl/test_graphql_app.py
tests/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
tests/**/*.py: Usehttpx_mockfixture for HTTP mocking in tests instead of making real HTTP requests
Do not add@pytest.mark.asynciodecorator to async test functions (async auto-mode is globally enabled)
Files:
tests/unit/ctl/test_graphql_app.py
tests/unit/**/*.py
📄 CodeRabbit inference engine (tests/AGENTS.md)
Unit tests must be fast, mocked, and have no external dependencies
Files:
tests/unit/ctl/test_graphql_app.py
🧬 Code graph analysis (1)
tests/unit/ctl/test_graphql_app.py (2)
infrahub_sdk/ctl/graphql.py (2)
find_gql_files(43-59)get_graphql_query(62-79)tests/helpers/cli.py (1)
remove_ansi_color(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: unit-tests (3.11)
- GitHub Check: unit-tests (3.14)
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.12)
- GitHub Check: documentation
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
tests/fixtures/unit/test_infrahubctl/graphql/invalid_query.gql (1)
1-9: Invalid query fixture correctly exercises validation failuresThe query is intentionally invalid against the provided schema and is well-suited for testing error handling in
get_graphql_queryand the CLI flows. No changes needed.tests/fixtures/unit/test_infrahubctl/graphql/valid_query.gql (1)
1-15: Valid query fixture aligns with test schema and covers key fieldsThe
GetTagsquery matches theBuiltinTagschema (arguments and nested fields) and provides a solid happy-path example for parsing and codegen tests. Looks good as-is.tests/fixtures/unit/test_infrahubctl/graphql/query_with_typename.gql (1)
1-16: Typename-heavy fixture provides good coverage for stripping logicIncluding
__typenameat root, edge, node, and nested field levels is ideal for testing that the typename-stripping utilities and CLI flows handle all nesting cases. No changes needed.tests/fixtures/unit/test_infrahubctl/graphql/test_schema.graphql (1)
1-47: Schema is coherent and consistent with query fixturesThe schema types and fields (especially
Query.BuiltinTag,PaginatedBuiltinTag.edges, andBuiltinTag/TextAttribute) line up cleanly with all the supplied fixtures, providing a solid, self-contained test schema. No issues spotted.tests/unit/ctl/test_graphql_app.py (1)
19-66: find_gql_files tests exercise core behaviors and edge cases wellThe tests cover single-file, directory, nested directory, nonexistent path, and empty-directory scenarios, matching the implementation of
find_gql_filesclosely. Assertions avoid relying on ordering (except for the single-file case, which is deterministic), so this looks solid.
Documentation
Improvements for generate-return-types ctl command
Summary by CodeRabbit
Documentation
Improvements
Tests
Fixtures / Schema
✏️ Tip: You can customize this high-level summary in your review settings.