-
-
Notifications
You must be signed in to change notification settings - Fork 110
Cli subcommand with annotation #579
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
Conversation
|
|
||
| class Settings(BaseSettings): | ||
| model_config = SettingsConfigDict(env_nested_delimiter='__') | ||
| animals: Annotated[Union[Annotated[Union[Cat, Dog], 'my_nested_annotation'], None], 'my_annotation'] |
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.
@hramezani I have the top level fix for the CLI in place. However, I discovered that the env parsing does not handle nested annotated unions properly. I've added a test case here to demonstrate the issue.
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 is a special case. I am sure that there are more cases that fail(we are not aware of them).
I would prefer keeping the code base simple and preventing breaking change instead of supporting all complex scenarios.
|
|
||
| # Check if annotation is of the form Union[type, ...]. | ||
| if typing_objects.is_union(origin): | ||
| return _union_is_complex(annotation, metadata) |
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.
@hramezani From what I observed, it appears to be related to incorrectly determining that nested annotated unions are not complex. This change fixed it, but causes other tests fail. I'm not sure if it is the correct fix. Can you take a look?
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.
Sorry @kschwab for the delay.
I am also not sure what the correct fix is here. As the change breaks some existing tests, I would prefer to remove it.
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.
No worries @hramezani. I'm ok with closing this as a won't fix as well.
|
Closing as won't fix due to maintenance and complexity concerns. |
Fixes #573