-
-
Notifications
You must be signed in to change notification settings - Fork 13
Fix subflow type #222
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
Fix subflow type #222
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the Flow.subflows field to use a dedicated Subflow TypedDict type instead of a recursive Flow model, restoring the previously correct typing that avoids Pydantic validation issues. Class diagram for updated Flow and Subflow typesclassDiagram
class BaseModel
class TypedDict
class Flow {
+str identifier
+str title
+str description
+str~dict~ metadata
+str language
+str dependencies
+list~Parameter~ parameter
+list~Subflow~ subflows
+list~str~ tag
}
class Subflow {
+str identifier
+Flow flow
}
Flow --|> BaseModel
Subflow --|> TypedDict
Flow o--> Subflow
Subflow o--> Flow
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe change replaces recursive self-referential typing in the Flow schema with an explicit TypedDict-based structure. A new public type Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
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.
Hey - I've left some high level feedback:
- Since
Subflowis referenced inFlowbefore it is defined, consider either moving theSubflowdefinition aboveFlowor changing the annotation to a string literal (list["Subflow"]) to avoid issues with tools/environments that don't enable postponed evaluation of annotations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `Subflow` is referenced in `Flow` before it is defined, consider either moving the `Subflow` definition above `Flow` or changing the annotation to a string literal (`list["Subflow"]`) to avoid issues with tools/environments that don't enable postponed evaluation of annotations.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 (1)
src/schemas/flows.py (1)
34-36: Consider using BaseModel for consistency and validation.The
SubflowTypedDict provides type hints but no runtime validation, unlike theFlowBaseModel. While this lighter approach might be intentional to avoid nested validation overhead, using a Pydantic BaseModel would ensure consistent validation across the schema and catch potential data issues earlier.🔎 Alternative using BaseModel
-class Subflow(TypedDict): +class Subflow(BaseModel): identifier: str | None flow: FlowNote: Keep this as TypedDict if the performance/simplicity trade-off is intentional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/schemas/flows.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/schemas/flows.py (1)
tests/conftest.py (2)
flow(79-89)Flow(70-75)
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
src/schemas/flows.py (2)
4-4: LGTM! Import change aligns with the structural shift.The import change from
SelftoTypedDictcorrectly supports the new explicit subflow structure.
28-28: Verify circular reference handling in Flow.subflows implementation.The change from
list[Self]tolist[Subflow]resolves Pydantic's handling of recursive type definitions by using a TypedDict intermediary. Note thatSubflowis a TypedDict containingidentifierand aflow: Flowfield—meaning subflows are validated as dictionaries that contain Flow BaseModel instances, not dictionaries alone.Verify that:
- Database queries properly construct
Subflowdictionaries with validFlowinstances- Serialization correctly handles the nested
Flowobjects within the TypedDict structure
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
=======================================
Coverage ? 54.44%
=======================================
Files ? 32
Lines ? 1135
Branches ? 100
=======================================
Hits ? 618
Misses ? 516
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I believe the typing is correct in That said, I made the change because I misremembered the role of |
Hmm, I haven't looked into the changes, but I updated my |
|
You're right. It does lead to an issue, perhaps I was loading a cached version on the page when I checked. I'll look into this and fix it or revert the change 👍 |
|
Reverted change in #225 |
The
developbranch was recently removed, and themainbranch is now used for both development and releases (possibly via a rename fromdeveloptomain). Thedevelopbranch previously had an issue with the type of subflows, which caused Pydantic errors; this issue was fixed onmain. However, that fix is no longer present onmain. This PR restores the fix.Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.