Fix decorators by ChJR · Pull Request #3783 · RustPython/RustPython
Conversation
Except test_expressions in test_decorators.py file.
Now it is working with full test_decorators.py test file.
test_socket.py needs an update; I can tackle that later.
|
|
||
| fn py_new(cls: PyTypeRef, callable: Self::Args, vm: &VirtualMachine) -> PyResult { | ||
| PyClassMethod { | ||
| let _callable = callable.clone(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually _ prefix means unused variable.
Comment on lines +81 to +84
| match obj.set_attr("__doc__", doc, vm) { | ||
| Err(e) => Err(e), | ||
| Ok(_) => result, | ||
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| match obj.set_attr("__doc__", doc, vm) { | |
| Err(e) => Err(e), | |
| Ok(_) => result, | |
| } | |
| obj.set_attr("__doc__", doc, vm)?; | |
| result |
|
|
||
| fn py_new(cls: PyTypeRef, callable: Self::Args, vm: &VirtualMachine) -> PyResult { | ||
| PyStaticMethod { callable } | ||
| let _callable = callable.clone(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is another _callable
Good points. I will change the codes.
Comment on lines +58 to +62
| let _descr_get: PyResult<PyObjectRef> = zelf.callable.lock().get_attr("__get__", vm); | ||
| match _descr_get { | ||
| Err(_) => Ok(PyBoundMethod::new_ref(cls, zelf.callable.lock().clone(), &vm.ctx).into()), | ||
| Ok(_descr_get) => vm.invoke(&_descr_get, (cls.clone(), cls)), | ||
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_descr_get is unnecessarily _ prefixed
Comment on lines +77 to +78
| let doc = vm.unwrap_pyresult(doc); | ||
| let obj = vm.unwrap_pyresult(result.clone()); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a correct error handling.
Both of them can be Err and they will panic.
| fn py_new(cls: PyTypeRef, callable: Self::Args, vm: &VirtualMachine) -> PyResult { | ||
| PyClassMethod { | ||
| callable: PyMutex::new(callable), | ||
| let result: PyResult<PyObjectRef> = PyClassMethod { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because PyResult = PyResult<PyObjectRef>, this is redundant.
| PyClassMethod { | ||
| callable: PyMutex::new(callable), | ||
| let result: PyResult<PyObjectRef> = PyClassMethod { | ||
| callable: PyMutex::new(callable.clone()), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to get __doc__ before making PyClassMethod to avoid clone.
| let result: PyResult<PyObjectRef> = PyClassMethod { | ||
| callable: PyMutex::new(callable.clone()), | ||
| } | ||
| .into_ref_with_type(vm, cls) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd like to propagate error here before later handling
| .into_ref_with_type(vm, cls) | |
| .into_ref_with_type(vm, cls)? |
Comment on lines +78 to +81
| match doc { | ||
| Err(_) => None, | ||
| Ok(doc) => Some(obj.set_attr("__doc__", doc, vm)), | ||
| }; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| match doc { | |
| Err(_) => None, | |
| Ok(doc) => Some(obj.set_attr("__doc__", doc, vm)), | |
| }; | |
| if let Ok(doc) = doc { | |
| obj.set_attr("__doc__", doc, vm)?; | |
| } |
set_attr result need to be checked with ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points!
Check 'Named Expressions Need Not Be Parenthesized' Section
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thank you for contributing!
oh, this is unexpected errors ok, that's solved by retry