- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
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. |
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.
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
Co-authored-by: Eric Jolibois <em.jolibois@gmail.com>
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.
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: |
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.
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
please update |
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. |
thanks so much. |
Change Summary
In the validate_arguments decorator the Field object can be used to provide additional information.
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.
Related issue number
With this change issue #2429 can be considered closed.
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)