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 individual partial enum values #1228

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mwildehahn
Copy link
Contributor

@mwildehahn mwildehahn commented Nov 30, 2024

If you use literal values with partial it fails as the model is generating partial values. PartialLiteralMixin is one approach to fixing this but then means no other string values will stream as they're processed.

This is another approach that doesn't change how we parse the JSON, but just means the partial model won't validate these enums until they're fully resolved.


Important

Adds support for handling partial enum values in JSON parsing using PartialLiteralValidator and updates tests accordingly.

  • Behavior:
    • Introduces PartialLiteralValidator in partial.py to handle partial enum values.
    • Updates _make_field_optional() to use PartialLiteralValidator for Literal types with Partial.
  • Tests:
    • Adds test_partial_enums() in test_partial.py to validate partial enum handling.
    • Ensures enums are not validated until fully resolved, except for non-partial fields.

This description was created by Ellipsis for 15d0093. 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 15d0093 in 47 seconds

More details
  • Looked at 135 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. instructor/dsl/partial.py:13
  • Draft comment:
    Any is imported twice. Remove the duplicate import on line 18.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statement for Any is duplicated, which is unnecessary and can be removed for cleaner code.
2. instructor/dsl/partial.py:55
  • Draft comment:
    Returning None for invalid literals might lead to unexpected behavior. Consider raising a validation error or handling this case explicitly.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PartialLiteralValidator class is designed to validate literal values, but it returns None if the value is not in the literal type. This could lead to unexpected behavior if not handled properly. It might be better to raise a validation error or handle this case explicitly.
3. instructor/dsl/partial.py:104
  • Draft comment:
    Consider adding a comment to explain the logic for handling Partial in field metadata when dealing with Literal types.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The PartialLiteralValidator is used to validate literal types, but the logic for handling Partial in field metadata is not clearly explained. A comment explaining this logic would improve code readability.
4. tests/dsl/test_partial.py:230
  • Draft comment:
    Typo in comment: 'partil' should be 'partial'.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The test case test_partial_enums has a typo in the comment on line 230. It should be 'partial' instead of 'partil'.
5. tests/dsl/test_partial.py:57
  • Draft comment:
    Assertions should include error messages for clarity and debugging purposes. This applies to all assertions in this file.
  • Reason this comment was not posted:
    Confidence changes required: 80%
    The assertion statements in the test file lack error messages, which are important for debugging when tests fail.
6. instructor/dsl/partial.py:60
  • Draft comment:
    Function names should follow a consistent naming pattern. Consider using snake_case for all function names to maintain consistency.
  • Reason this comment was not posted:
    Confidence changes required: 70%
    The function names in the code are inconsistent. Some functions use snake_case while others use camelCase. Consistent naming conventions should be followed.

Workflow ID: wflow_NMW6RGhC1ZPqHg3J


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

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.

1 participant