Remove remaining usages of arg_check! #1953
Conversation
|
I'm planning to look into parameter-based arg checking. Any pointers are appreciated! |
|
I've reproduced the CI failure on my machine, will look into it! |
vm/src/obj/objbool.rs
Outdated
| optional = [(val, None)] | ||
| ); | ||
| let value = match val { | ||
| fn tp_new(vm: &VirtualMachine, mut args: PyFuncArgs) -> PyResult { |
There was a problem hiding this comment.
| fn tp_new(vm: &VirtualMachine, mut args: PyFuncArgs) -> PyResult { | |
| fn tp_new(zelf: PyObjectRef, value: PyObjectRef, vm: &VirtualMachine) -> PyResult { |
Unless there are keyword arguments, this form is more easy for most of cases. In this way, you don't need to unpack zelf and value from args yourself.
There was a problem hiding this comment.
Is there another form I could use when I need to accept a zero-arguments case?
There was a problem hiding this comment.
(args..., vm: &VirtualMachine) -> ReturnType form work for everything.
These examples are actual RustPython code for zero-argument:
fn builtin_globals(vm: &VirtualMachine) -> PyResult<PyDictRef> { }
fn repr(self, vm: &VirtualMachine) -> PyResult<String> { }If you are looking for optional arguments that would allow zero-arguments for users, check for OptionalArg for positional-only argument
fn sys_exit(code: OptionalArg<PyObjectRef>, vm: &VirtualMachine) -> PyResultand search for FromArgs for keyword arguments.
There was a problem hiding this comment.
I tried using OptionalArg for tp_new, but it seems that it's not suitable for tp_new at the moment, since it allows keyword arguments to be passed as well. For example, when used in this form:
fn tp_new(zelf: PyObjectRef, x: OptionalArg<PyObjectRef>, vm: &VirtualMachine)
It fails the test test_keyword_args in test_bool.py.
There was a problem hiding this comment.
Oh, that's fine, just remove the unexpectedFailure annotation from test_keyword_args. I don't think it should actually allow keyword args, it uses take_positional under the hood.
There was a problem hiding this comment.
@coolreader18 I'm not sure if I used OptionalArg correctly, but I've added the related commits!
|
Thanks for contributing! |
| let iterable = args.take_positional(); | ||
| if iterable.is_none() { | ||
| return Err(vm.new_type_error("sorted expected 1 arguments, got 0".to_string())); | ||
| } | ||
| let items = vm.extract_elements(&iterable.unwrap())?; |
There was a problem hiding this comment.
Though it is already merged, we conventionally prefer to use this form to avoid unwrap when possible:
| let iterable = args.take_positional(); | |
| if iterable.is_none() { | |
| return Err(vm.new_type_error("sorted expected 1 arguments, got 0".to_string())); | |
| } | |
| let items = vm.extract_elements(&iterable.unwrap())?; | |
| let iterable = args.take_positional().unwrap_or_else(|| Err(vm.new_type_error("sorted expected 1 arguments, got 0".to_string())))?; | |
| let items = vm.extract_elements(&iterable)?; |
This pull request removes the remaining usages of
arg_check!and closes #1714.