-
Notifications
You must be signed in to change notification settings - Fork 77
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 KeyError in _validate_and_extract_signature_types and Add Tests for Pydantic Actions #403
base: main
Are you sure you want to change the base?
Conversation
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.
👍 Looks good to me! Reviewed everything up to 5a91b6b in 18 seconds
More details
- Looked at
62
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. burr/integrations/pydantic.py:117
- Draft comment:
Consider checking ifstate_model
is a class before usingissubclass
to avoid potentialTypeError
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code change already includes a check forstate_model
beingNone
orinspect.Parameter.empty
, which should prevent aTypeError
when usingissubclass
. The comment may be redundant because the code change seems to handle the potential issue. The comment does not provide strong evidence of a necessary change beyond what is already implemented.
I might be overlooking a scenario wherestate_model
could be a non-class type that is neitherNone
norinspect.Parameter.empty
, which could still cause aTypeError
. However, the current checks seem comprehensive for the context provided.
The checks forNone
andinspect.Parameter.empty
should cover the cases wherestate_model
is not a class, as these are the likely non-class values in this context.
The comment is likely unnecessary because the code change already addresses the potentialTypeError
by checking forNone
andinspect.Parameter.empty
. The comment should be deleted.
Workflow ID: wflow_TRgRGX4yohQHltuv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
A few points on this -- thanks for the addition!
raise ValueError( | ||
f"Function fn: {fn.__qualname__} is not a valid pydantic action. " | ||
"a type annotation of a type extending: pydantic.BaseModel. Got parameter " | ||
"state: {state_model.__qualname__}." | ||
"The 'state' parameter must be annotated with a type extending pydantic.BaseModel." |
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.
This removes information -- we should print what it actually is
@@ -154,27 +154,27 @@ def test_model_from_state(): | |||
|
|||
|
|||
def _fn_without_state_arg(foo: OriginalModel) -> OriginalModel: | |||
... | |||
return foo; |
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.
Why remove the ...? We never call them.
PR fixes a
KeyError
issue in_validate_and_extract_signature_types
when the"state"
key is missing in type hints. It also has error handling and adds unit tests.Changes
_validate_and_extract_signature_types
to use.get()
test_burr_pydantic.py
.How I tested this
Added new tests for both error and success cases. Below are the test details:
Notes
ensures better debugging and avoids cryptic errors , closed #385
Checklist
Important
Fix
KeyError
in_validate_and_extract_signature_types
and add tests for Pydantic actions.KeyError
in_validate_and_extract_signature_types
inpydantic.py
by using.get()
for "state" key.test_burr_pydantic.py
for_validate_and_extract_signature_types
to cover missing "state" key and incorrect return types.test_burr_pydantic.py
with valid logic for testing.This description was created by for 5a91b6b. It will automatically update as commits are pushed.