add other bytes method #847
Conversation
move trait IsByte to ByteOr refactor center refactor count fix center with negative value add test for count(integer)
Codecov Report
@@ Coverage Diff @@
## master #847 +/- ##
==========================================
+ Coverage 62.89% 64.21% +1.31%
==========================================
Files 88 88
Lines 14547 15172 +625
Branches 3288 3405 +117
==========================================
+ Hits 9150 9743 +593
+ Misses 3238 3178 -60
- Partials 2159 2251 +92
Continue to review full report at Codecov.
|
|
I have : split and rsplit, expandstab , partition/reparition, spitlines, zfill, replace |
|
I think maybe save that for another PR, yes. |
vm/src/obj/objbyteinner.rs
Outdated
| #[pyarg(positional_only, optional = false)] | ||
| sub: PyObjectRef, | ||
| #[pyarg(positional_only, optional = true)] | ||
| start: OptionalArg<PyObjectRef>, |
There was a problem hiding this comment.
It could be Int or None so I have to accept PyObjectRef
There was a problem hiding this comment.
Can you do OptinalArg<PyIntRef>?
There was a problem hiding this comment.
Or maybe OptionalArg<Option<PyIntRef>>? I'm not sure if Option is FromArgs yet, but it should be the specified type or Python None.
There was a problem hiding this comment.
using OptionalArg<Option<PyIntRef>>, you mean this implementation in impl block (line 126)?
let start = match self.start {
OptionalArg::Present(Some(int)) => Some(int.as_bigint().clone()),
_ => None,
};
There was a problem hiding this comment.
I let it for now.
let start = match self.start {
OptionalArg::Present(Some(int)) => Some(int.as_bigint().clone()),
_ => None,
};instead of
let start = match self.start.into_option() {
Some(Some(int)) => Some(int.as_bigint().clone()),
_ => None,
};using into_option() isn't much shorter and I find it less explicit. what do you think ?
There was a problem hiding this comment.
They both look great to me.
There was a problem hiding this comment.
The rust clippy tool would probably suggest to use something like this:
let start = if let Some(Some(int)) = self.start.into_option() {
Some(int.as_bigint().clone())
} else {
None
};There was a problem hiding this comment.
I use clippy and non suggestion about it
There was a problem hiding this comment.
It's especially handy when you have a match with only two options (basically an if-else case).
|
Sorry for the late response @jgirardet. I will try and finish going over all the PR soon. |
|
np @palaviv it's far better like it. |
|
This looks way better :). |
|
PyByteInner doesn't implement PyValue so I can't use the default Either |
|
I think this change is ready for merge now? Thank you for reviewing the change. |
some refactoring
some more methods