Add Naming & Data Models for Custom Deployment SDK#111
Conversation
This phase adds: 1. Naming utilities (src/prefect/_sdk/naming.py): - to_identifier(): Convert arbitrary names to valid Python identifiers - to_class_name(): Convert names to PascalCase class names - Unicode separator handling (em-dash, non-breaking space become word boundaries) - NFKD normalization for accented characters (é → e) - Python keyword handling (class → class_ for identifiers, Class for class names) - Reserved name detection for SDK surface (run, run_async, with_options, etc.) - Collision resolution with numeric suffixes (_2, _3, etc.) 2. Data models (src/prefect/_sdk/models.py): - WorkPoolInfo: Work pool name, type, job variables schema - DeploymentInfo: Deployment name, flow name, parameter schema, work pool ref - FlowInfo: Flow name with list of deployments - SDKGenerationMetadata: Generation time, Prefect version, workspace, API URL - SDKData: Complete container for SDK generation with convenience methods 3. Comprehensive tests (227 total for _sdk module): - Edge cases: emoji, Unicode, keywords, empty strings, collisions - Non-ASCII separators, German ß, Unicode digits - Deterministic ordering verification Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Code Review by Qodo (Alpha)
1. `deployment_names` returns wrong values
|
| def deployment_names(self) -> list[str]: | ||
| """List of all deployment full names (derived from flows). | ||
|
|
||
| Returns names sorted alphabetically for deterministic code generation. | ||
| """ | ||
| names: list[str] = [] | ||
| for flow in self.flows.values(): | ||
| for deployment in flow.deployments: | ||
| names.append(deployment.name) | ||
| return sorted(names) |
There was a problem hiding this comment.
1. deployment_names returns wrong values 📘 Rule Violation
• SDKData.deployment_names is documented as returning deployment full_name values, but it actually appends deployment.name, making the property’s behavior inconsistent with its own docstring. • This is misleading/self-non-documenting and can cause downstream code generation to use incomplete identifiers (deployment-only names) instead of flow/deployment names. • The mismatch also undermines deterministic output expectations because the returned values are not the intended ones.
Agent Prompt
## Issue description
`SDKData.deployment_names` claims to return deployment full names, but it currently appends `deployment.name`.
## Issue Context
This makes the code misleading and can break downstream consumers expecting `flow-name/deployment-name` strings.
## Fix Focus Areas
- src/prefect/_sdk/models.py[120-129]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Check if base is available | ||
| if base not in existing or base not in reserved: | ||
| return base |
There was a problem hiding this comment.
2. make_unique_identifier uses or 📘 Rule Violation
• make_unique_identifier() is intended to enforce uniqueness and reserved-name avoidance, but its availability check uses or, causing it to return base even when it collides with existing or is in reserved. • This breaks collision handling and reserved-name avoidance, creating edge-case failures where generated identifiers are not unique or conflict with reserved SDK surface names. • Because this failure can occur silently, it increases the risk of producing invalid/ambiguous generated SDK output without obvious error signals.
Agent Prompt
## Issue description
`make_unique_identifier()` incorrectly uses `or` when deciding whether `base` is available, allowing collisions or reserved names to slip through.
## Issue Context
This function is responsible for ensuring generated identifiers are both unique and do not conflict with reserved SDK surface names.
## Fix Focus Areas
- src/prefect/_sdk/naming.py[253-263]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| def test_all_reserved_in_module_context(self): | ||
| """Test that 'all' is reserved (normalized form of '__all__').""" | ||
| from prefect._sdk.naming import RESERVED_MODULE_IDENTIFIERS | ||
|
|
There was a problem hiding this comment.
3. Deferred import in test 📘 Rule Violation
• A new import is introduced inside test_all_reserved_in_module_context() without an obvious need (no circular import avoidance or optional dependency handling). • This reduces predictability/tooling compatibility and violates the project guidance to keep imports at module scope unless required.
Agent Prompt
## Issue description
A deferred import was added inside a test function without a clear need.
## Issue Context
Keeping imports at module scope improves readability and tooling support.
## Fix Focus Areas
- tests/_sdk/test_naming.py[1-14]
- tests/_sdk/test_naming.py[300-305]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Benchmark PR from qodo-benchmark#365