Revert change of removing subflow context #225
Conversation
The extra nesting provided a nickname of the subflow in the context of the parent flow.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pull request introduces a new Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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
Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
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:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
Flowmodel referencesSubflowbefore it is defined; consider either movingSubflowaboveFlowor using a string-annotated type (e.g.subflows: list["Subflow"]) to avoid forward-reference issues with type checkers and Pydantic’s annotation evaluation. - In
get_flow, you now build subflow dictionaries manually; consider constructingSubflow-typed structures (or a Pydantic model) consistently instead of raw dicts to reduce the risk of key/name mismatches across the router, schema, and tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Flow` model references `Subflow` before it is defined; consider either moving `Subflow` above `Flow` or using a string-annotated type (e.g. `subflows: list["Subflow"]`) to avoid forward-reference issues with type checkers and Pydantic’s annotation evaluation.
- In `get_flow`, you now build subflow dictionaries manually; consider constructing `Subflow`-typed structures (or a Pydantic model) consistently instead of raw dicts to reduce the risk of key/name mismatches across the router, schema, and tests.
## Individual Comments
### Comment 1
<location> `tests/routers/openml/flows_test.py:224-233` </location>
<code_context>
+ "identifier": None,
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case that covers subflows with a non-None identifier
This test only exercises `identifier=None`, while the expectation now treats each subflow as an object with both `identifier` and a nested `flow`. Please add or extend a test to cover a subflow with a non-empty identifier so we verify it is returned correctly and not ignored or mis-serialized.
Suggested implementation:
```python
"identifier": "weka.J48",
```
If you also want to explicitly keep coverage for `identifier=None`, you can either:
1. Duplicate this subflow entry and keep one with `"identifier": None` and another with a non-empty identifier, or
2. Add a separate test case that asserts the `identifier` is `None` for a different subflow in the response.
Those additional changes would need to align with the actual API response data your fixtures/mocks are producing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "identifier": None, | ||
| "flow": { | ||
| "id": 4, | ||
| "uploader": 16, | ||
| "name": "weka.J48", | ||
| "class_name": "weka.classifiers.trees.J48", | ||
| "version": 1, | ||
| "external_version": "Weka_3.9.0_11194", | ||
| "description": ( | ||
| "Ross Quinlan (1993). C4.5: Programs for Machine Learning. " |
There was a problem hiding this comment.
suggestion (testing): Add a test case that covers subflows with a non-None identifier
This test only exercises identifier=None, while the expectation now treats each subflow as an object with both identifier and a nested flow. Please add or extend a test to cover a subflow with a non-empty identifier so we verify it is returned correctly and not ignored or mis-serialized.
Suggested implementation:
"identifier": "weka.J48",If you also want to explicitly keep coverage for identifier=None, you can either:
- Duplicate this subflow entry and keep one with
"identifier": Noneand another with a non-empty identifier, or - Add a separate test case that asserts the
identifierisNonefor a different subflow in the response.
Those additional changes would need to align with the actual API response data your fixtures/mocks are producing.
The extra nesting provided a nickname of the subflow in the context of the parent flow.
Supersedes #222