-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Alias support in decorator validate_arguments #3019
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
|
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. |
PrettyWood
left a comment
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 <[email protected]>
samuelcolvin
left a comment
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>.mdfile added describing change(see changes/README.md for details)