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.

Sign up for GitHub

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

Jump to bottom

Add support for field alias in dataclass signature #8387

Merged
merged 3 commits into from Dec 20, 2023
Merged

Add support for field alias in dataclass signature #8387

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9b73703
Fixing #8367
NeevCohen Dec 16, 2023
ab9ae9f
Change helper function name
NeevCohen Dec 19, 2023
f5de0da
Remove redundant if check
NeevCohen Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 12 additions & 5 deletions pydantic/_internal/_constructor_signature_generators.py
View file
Open in desktop
Expand Up @@ -10,6 +10,13 @@
from ..fields import FieldInfo


def field_to_param_name(field_name: str, field: FieldInfo) -> str:
"""Convert field to parameter name.
Returns the name to use for the parameter, giving priority to the alias of the field.
"""
Copy link
Member

Choose a reason for hiding this comment

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

I like that you've consolidated shared logic, but let's come up with a more descriptive name for this function, specifically relating to the fact that we're selecting either the alias or field name for the signature's param name.

Copy link
Member

Choose a reason for hiding this comment

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

Let's also make this private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After considering your previous comments, I don't think that I should duplicate the logic for extracting the alias since it is implemented in the process_param_defaults() function. Perhaps I should use that function or somehow extract the shared logic between that function and this one. Check out my comment on your other comments.

return field.alias if isinstance(field.alias, str) and is_valid_identifier(field.alias) else field_name


def generate_pydantic_signature(
init: Callable[..., None],
fields: dict[str, FieldInfo],
Expand Down Expand Up @@ -37,6 +44,9 @@ def generate_pydantic_signature(
for param in islice(present_params, 1, None): # skip self arg
# inspect does "clever" things to show annotations as strings because we have
# `from __future__ import annotations` in main, we don't want that
if fields.get(param.name) and isinstance(fields[param.name].alias, str):
param_name = field_to_param_name(param.name, fields[param.name])
param = param.replace(name=param_name)
Copy link
Member

Choose a reason for hiding this comment

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

I don't love mutating the param object here. I think we could find a better solution that's less redundant...

Copy link
Member

Choose a reason for hiding this comment

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

I'll note, the parameter_post_processor function for the dataclass signature generation also has alias extraction logic, but that only applies when we have a dataclass field using pydantic's Field as a default.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I see why you've modified the param.name here... perhaps that is fine for now then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love mutating the param object here. I think we could find a better solution that's less redundant...

Yeah it seemed iffy to me too, but the that's what the existing code did so I preferred to stick it. Do you think I should save it in a different variable?

Copy link
Contributor Author

@NeevCohen NeevCohen Dec 18, 2023 •
edited

Choose a reason for hiding this comment

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

I'll note, the parameter_post_processor function for the dataclass signature generation also has alias extraction logic, but that only applies when we have a dataclass field using pydantic's Field as a default.

Yeah I just looked at process_param_defaults() which gets passed as parameter_post_processor. I thought perhaps I should just add the logic there and avoid the duplication of the alias extraction logic.

Perhaps adding another parameter to it like so

def process_param(param: Parameter, field: Optional[FieldInfo] = None) -> Parameter:
    """Modify the signature for a parameter in a dataclass. If a parameter has a Model field associated with it, use it for modifying the parameter.

    Args:
        param (Parameter): The parameter
        field: An optional Model field associated with the parameter

    Returns:
        Parameter: The custom processed parameter
    """
    # Logic that uses the field if it exists or the parameter default
    ....

Do you think this is good solution?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I like the thought, though overall I'm tempted to not increase the complexity of that function.

Why don't we stick with something similar to your original solution, then do a more significant refactoring to fix #7978 in a separate PR? Could you make the helper function that you've designed private, for now?

As a side note, looks like we changed up the logic for the signature pattern in this PR, which did represent a major reduction in redundancy across signature generation logic: https://github.com/pydantic/pydantic/pull/7925/files.

if param.annotation == 'Any':
param = param.replace(annotation=Any)
if param.kind is param.VAR_KEYWORD:
Expand All @@ -48,16 +58,13 @@ def generate_pydantic_signature(
allow_names = config_wrapper.populate_by_name
for field_name, field in fields.items():
# when alias is a str it should be used for signature generation
if isinstance(field.alias, str):
param_name = field.alias
else:
param_name = field_name
param_name = field_to_param_name(field_name, field)

if field_name in merged_params or param_name in merged_params:
continue

if not is_valid_identifier(param_name):
if allow_names and is_valid_identifier(field_name):
if allow_names:
param_name = field_name
else:
use_var_kw = True
Expand Down
3 changes: 2 additions & 1 deletion tests/test_dataclasses.py
View file
Open in desktop
Expand Up @@ -2487,9 +2487,10 @@ class Model:
a: float = dataclasses.field(default_factory=float)
b: float = Field(default=1.0)
c: float = Field(default_factory=float)
d: int = dataclasses.field(metadata={'alias': 'dd'}, default=1)

assert str(inspect.signature(Model)) == (
"(x: int, y: str = 'y', z: float = 1.0, a: float = <factory>, b: float = 1.0, c: float = <factory>) -> None"
"(x: int, y: str = 'y', z: float = 1.0, a: float = <factory>, b: float = 1.0, c: float = <factory>, dd: int = 1) -> None"
)


Expand Down