Skip to content
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

Support nested generics with partial #1207

Merged
merged 3 commits into from
Nov 23, 2024

Conversation

mwildehahn
Copy link
Contributor

@mwildehahn mwildehahn commented Nov 22, 2024

See the test case for an example of where this was failing before. This adds support for handling any level of nesting for generic types.


Important

Adds support for nested generics in Partial class with new _process_generic_arg() function and updates tests for verification.

  • Behavior:
    • Adds support for nested generics in Partial class by introducing _process_generic_arg() in partial.py.
    • Handles any level of nesting for generic types like Union, List, Dict.
    • Special handling for Union types with types.UnionType for Python 3.10+.
  • Tests:
    • Adds test_union_with_nested() in test_partial.py to verify nested generics handling.
    • Updates existing tests to ensure compatibility with new nested generics support.

This description was created by Ellipsis for c9be3ba. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to c9be3ba in 1 minute and 11 seconds

More details
  • Looked at 165 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. tests/dsl/test_partial.py:189
  • Draft comment:
    The test test_union_with_nested lacks an assertion to verify the expected behavior. Consider adding an assertion to ensure the Partial model behaves as expected.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The test case test_union_with_nested is missing an assertion to verify the expected behavior of the Partial model. Without an assertion, the test does not confirm the correctness of the code.
2. instructor/dsl/partial.py:50
  • Draft comment:
    Add comments to _process_generic_arg to explain its logic, especially since it deals with complex type manipulations.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function _process_generic_arg is a new addition to handle nested generics, but it lacks comments explaining its logic, especially since it deals with complex type manipulations.
3. instructor/dsl/partial.py:81
  • Draft comment:
    Add comments to _make_field_optional to explain its logic, especially since it deals with complex type manipulations.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function _make_field_optional is modified to use _process_generic_arg, but it lacks comments explaining its logic, especially since it deals with complex type manipulations.
4. instructor/dsl/partial.py:392
  • Draft comment:
    Add comments to _wrap_models to explain its logic, especially since it deals with complex type manipulations.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The function _wrap_models is modified to use _process_generic_arg, but it lacks comments explaining its logic, especially since it deals with complex type manipulations.

Workflow ID: wflow_WpQzbHuk6UcXE1U3


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@jxnl jxnl requested a review from ivanleomk November 22, 2024 22:08
@jxnl jxnl merged commit 934c728 into instructor-ai:main Nov 23, 2024
0 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants