-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
adds model_name option in ConfigDict #6814
Changes from 3 commits
25aea57
0fd7fe2
090985b
e8af284
5b32e4f
48e2068
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,7 @@ | |
from pydantic.errors import PydanticUserError | ||
from pydantic.fields import FieldInfo | ||
from pydantic.type_adapter import TypeAdapter | ||
from pydantic.v1.schema import get_model_name_map | ||
from pydantic.warnings import PydanticDeprecationWarning | ||
|
||
if sys.version_info < (3, 9): | ||
|
@@ -492,6 +493,66 @@ def my_function(): | |
pass | ||
|
||
|
||
def test_config_model_name() -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I'm missing something here, but isn't the only thing that we really need to focus on testing just that the name of the model when assigned via You'll definitely want to run a test or two with model names not equivalent to what they would by default be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The goal of this PR was to address #6304, but considering this comment, maybe it is better to not touch the I would rework a bit the implementation in this direction and keep this test. What do you think? |
||
CLIENT_USER_MODEL_NAME = 'ClientUser' | ||
BUSINESS_USER_MODEL_NAME = 'BusinessUser' | ||
|
||
def _get_business_user_class(): | ||
class User(BaseModel): | ||
model_config = ConfigDict(model_name=BUSINESS_USER_MODEL_NAME) | ||
|
||
return User | ||
|
||
def _get_client_user_class(): | ||
class User(BaseModel): | ||
model_config = ConfigDict(model_name=CLIENT_USER_MODEL_NAME) | ||
|
||
return User | ||
|
||
BusinessUser = _get_business_user_class() | ||
ClientUser = _get_client_user_class() | ||
|
||
name_map = get_model_name_map({BusinessUser, ClientUser}) | ||
assert name_map[BusinessUser] == BUSINESS_USER_MODEL_NAME | ||
assert name_map[ClientUser] == CLIENT_USER_MODEL_NAME | ||
|
||
assert BusinessUser().model_json_schema()['title'] == BUSINESS_USER_MODEL_NAME | ||
assert ClientUser().model_json_schema()['title'] == CLIENT_USER_MODEL_NAME | ||
|
||
|
||
def test_config_model_name__long_model_name_on_conflict() -> None: | ||
CLIENT_USER_MODEL_NAME = 'ClientUser' | ||
|
||
def _get_client_user_class(): | ||
class User(BaseModel): | ||
model_config = ConfigDict(model_name=CLIENT_USER_MODEL_NAME) | ||
|
||
return User | ||
|
||
def _get_client_2_user_class(): | ||
class User(BaseModel): | ||
model_config = ConfigDict(model_name=CLIENT_USER_MODEL_NAME) | ||
|
||
return User | ||
|
||
ClientUser = _get_client_user_class() | ||
ClientUserV2 = _get_client_2_user_class() | ||
|
||
name_map = get_model_name_map({ClientUser, ClientUserV2}) | ||
assert ( | ||
name_map[ClientUser] | ||
== 'tests__test_config__test_config_model_name__long_model_name_on_conflict__<locals>___get_client_user_class__<locals>__User' | ||
) | ||
assert ( | ||
name_map[ClientUserV2] | ||
== 'tests__test_config__test_config_model_name__long_model_name_on_conflict__<locals>___get_client_2_user_class__<locals>__User' | ||
) | ||
|
||
# FIXME: Maye the model_json_schema should have a long name on conflict? | ||
assert ClientUser().model_json_schema()['title'] == CLIENT_USER_MODEL_NAME | ||
assert ClientUserV2().model_json_schema()['title'] == CLIENT_USER_MODEL_NAME | ||
|
||
|
||
def test_multiple_inheritance_config(): | ||
class Parent(BaseModel): | ||
model_config = ConfigDict(frozen=True, extra='forbid') | ||
|
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.
Would it be possible to have some kind of "backup" of the original class name if overridden by
config.model_name
? I'm making use ofcls.__name__
here, and comparing it to AST-parsed file.In fact it would be great to have this PR merged before #6563, I will then be able to rebase on top of it and add this "backup" field
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.
I think that if
cls.__name__
is used somewhere else, maybe it would be better to keep its original behaviour and create an extra attribute for the model_name that will explicitly interact with thev1.schema
functions. What do you think?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.
Yes this seems better:
__name__
is a Python implementation detail and people should expect it to be the same as defined in the source code (it might also be used by documentation tools). Perhaps you can define a specific attribute that will be used for schema generation