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

validate_arguments decorator example implementation #1263

Closed
wants to merge 16 commits into from

Conversation

BLooperZ
Copy link

@BLooperZ BLooperZ commented Feb 27, 2020

This is an alternative implementation for the validate_arguments decorator #1179
Which also happen to fix #1222
(It also does not relay on arguments naming conventions)

I would not recommend merging this as it does not wrap raised TypeError as ValidationError (making relevant tests fail).
But might be useful for extracting logic and finding out what causes the above issue.

@BLooperZ
Copy link
Author

BLooperZ commented Feb 28, 2020

Update:
Now this implementation also support wrapping TypeError in ValidationError.
Notable difference is that validation for calling the function (TypeError) happens on separate stage, so those errors won't mix with value errors (which raised after the model has instantiated)

I have edited the tests to highlight the differences, please have a look.
(String annotation is failing)

@samuelcolvin
Copy link
Member

Still seems to be failing, also I'm not sure what the advantages of this approach are?

I've provided a simpler solution to #1222 in #1272, please let me know if that's acceptable.

@BLooperZ
Copy link
Author

BLooperZ commented Mar 2, 2020

is there something else failing beside string annotations? (I have left this test case active to reflect that - that's what keep failing in CI)

I liked in this approach to have python itself bind the arguments
(SignatureCheck model is only used to wrap error message around signature.bind - can ignore)

and that we don't have to rely on nor restrict arguments names,
that's achieved by using 2 step approach for input validation:
calling -> throw error if the function can't be called the way it was called (too many positional arguments, multiple values for same argument, unexpected keyword, positional only passed as keyword) +check what was the error only if there was any (ask forgiveness not permission).
values -> arguments are not valid for annotated types (pydantic's BaseModel)

The logic part is pretty straightforward (getting arguments by kind -> check bindings -> validate arguments on matching model -> call function with validated arguments) - from entry to model creation is just transformation over a mapping, and the apply wrapper itself, which does signature check and validation is also very easy to understand and short.

of course you may decide whatever works, so all is acceptable, but it is also worth to look at this (I suggest looking the file decorator.py itself, not the diff).

Thank you for your response and this amazing project.

@samuelcolvin
Copy link
Member

This looks like an interesting approach, but how does it effect performance?

I guess we need some kind of benchmark to compare it with. If you don't have time, I'll try to get around to building a benchmark when I have more time.

@samuelcolvin
Copy link
Member

in the interests of getting the number of open PRs down to a manageable level, I'm closing this but could be reopened if performance benchmarks show it's a more performant approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@validate_arguments on instance methods
2 participants