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

Alias support in decorator validate_arguments #3019

Merged
merged 13 commits into from
Aug 8, 2022

Conversation

MAD-py
Copy link
Contributor

@MAD-py MAD-py commented Jul 25, 2021

Change Summary

In the validate_arguments decorator the Field object can be used to provide additional information.

@validate_arguments
def how_many(num: Annotated[int, Field(gt=10)]):
    return num

Now the Field alias parameter will also work in the decorator, the idea is to be able to add this functionality that exists in the creation of the models and did not work so far in the decorator.

@validate_arguments
def foo(bar: Annotated[str, Field(alias="quux")]):
    print(bar)

foo(quux=42)

Related issue number

With this change issue #2429 can be considered closed.

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Sorry, something went wrong.

@MAD-py
Copy link
Contributor Author

MAD-py commented Jul 25, 2021

Please review the solution I gave to issue #2429 when you have time.

I remain attentive to any correction or any questions you would have about what I did.

Copy link
Collaborator

@PrettyWood PrettyWood left a comment

Choose a reason for hiding this comment

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

Could you please add at least a second parameter on one of the functions in your test?
Other than that looks good to me for the code 👍 Please update
I'm surprised on the need though. I feel like having an alias for a function is very weird x) mypy will complain and it's hard to read. I though the need would be for constraints on range, regex and stuff only

@PrettyWood PrettyWood added awaiting author revision awaiting changes from the PR author and removed ready for review labels Sep 4, 2021
MAD-py and others added 2 commits September 4, 2021 12:33

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

and as @PrettyWood requested, please can we have more complete/challenging tests.

non_var_fields = set(self.model.__fields__) - {self.v_args_name, self.v_kwargs_name}
for k, v in kwargs.items():
if k in non_var_fields:
if k in non_var_fields or k in fields_alias:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if k in non_var_fields or k in fields_alias:
if k in non_var_fields or k in fields_alias is not None:

see #4252

@samuelcolvin
Copy link
Member

please update

@github-actions github-actions bot assigned MAD-py and unassigned samuelcolvin and PrettyWood Aug 4, 2022
@MAD-py
Copy link
Contributor Author

MAD-py commented Aug 5, 2022

Hi @samuelcolvin.

This would be the update of the tests and the integration of the empty string like a possible alias is not necessary because it is already built in as the tests show.

@samuelcolvin samuelcolvin merged commit 7431683 into pydantic:master Aug 8, 2022
@samuelcolvin
Copy link
Member

thanks so much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants