Add str.__rmod__() method. #190 #1262
Conversation
tests/snippets/strings.py
Outdated
| str.__rmod__(30, '%i') | ||
| except TypeError: | ||
| typeError = True | ||
| assert typeError |
There was a problem hiding this comment.
You can use assertRaises from testutils for this:
from testutils import assertRaises
...
with assertRaises(TypeError):
...There was a problem hiding this comment.
Thank you for sharing. I will use your proposal for this PR.
vm/src/obj/objstr.rs
Outdated
| match do_cformat(vm, _format_string, values.clone()) { | ||
| Ok(_format_string) => Ok(vm.new_str("NotImplemented".to_string())), | ||
| Err(err) => Err(err) | ||
| } |
There was a problem hiding this comment.
I think you misunderstood what you were seeing in the python shell -- in CPython, str.__rmod__(str, int) doesn't return "NotImplemented", it returns the actual value NotImplemented, which you can access from Rust as vm.ctx.not_implemented().
There was a problem hiding this comment.
I misunderstood that. I will fix and re-open this pull request.
|
I improved PR code quality. |
vm/src/obj/objstr.rs
Outdated
| fn rmod(&self, values: PyObjectRef, vm: &VirtualMachine) -> PyResult { | ||
| let format_string_text = &self.value; | ||
| let _format_string = CFormatString::from_str(format_string_text) | ||
| .map_err(|err| vm.new_value_error(format!("{}", err)))?; |
There was a problem hiding this comment.
| .map_err(|err| vm.new_value_error(format!("{}", err)))?; | |
| .map_err(|err| vm.new_value_error(err.to_string()))?; |
There was a problem hiding this comment.
That code found on str.mod too. I suggest that code should be moved to another PR.
There was a problem hiding this comment.
Sounds good, thanks for contributing!
vm/src/obj/objstr.rs
Outdated
| .map_err(|err| vm.new_value_error(format!("{}", err)))?; | ||
|
|
||
| match do_cformat(vm, _format_string, values.clone()) { | ||
| Ok(_format_string) => Ok(vm.ctx.not_implemented()), |
There was a problem hiding this comment.
Is the call to do_cformat required here? Or can we just always return NotImplemented?
There was a problem hiding this comment.
I have just checked your second suggestion, and I found it works.
I used do_cformat just for argument checking, but that method is not required.
I will add a new commit for this PR.
|
@ChJR there is a tiny style glitch. There is a clippy result in travis. |
CPython returns "NotImplemented" string when arguments supplied.