Allow try_update_quantity_from_tuple to set left alignment when needed #4766
Conversation
This test no longer fails
vm/src/cformat.rs
Outdated
| @@ -204,12 +204,18 @@ fn spec_format_string( | |||
| fn try_update_quantity_from_element( | |||
There was a problem hiding this comment.
Now quantity and precision has different spec, but they are handled by same function.
I don't think we necessarily need to split it to 2 functions, but adding a comment to try_update_quantity_from_element will be helpful to prevent confusion.
my suggestion is:
| fn try_update_quantity_from_element( | |
| # This functions is used to parse both quantity and precision. Please consider to split this function once they doesn't fit in single function anymore. | |
| fn try_update_quantity_from_element( |
There was a problem hiding this comment.
Thanks for your feedback! Yeah, I think that would help ease the potential confusion.
common/src/cformat.rs
Outdated
| @@ -754,6 +754,12 @@ impl CFormatString { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug, Copy, Clone, PartialEq)] | |||
There was a problem hiding this comment.
along with YunWon's suggestion, I'd document this. We currently don't do this as much as we'd probably like but it's relatively straight-forward here.
vm/src/cformat.rs
Outdated
| @@ -239,7 +249,7 @@ fn try_update_precision_from_tuple<'a, I: Iterator<Item = &'a PyObjectRef>>( | |||
| let Some(CFormatPrecision::Quantity(CFormatQuantity::FromValuesTuple)) = p else { | |||
| return Ok(()); | |||
| }; | |||
| let quantity = try_update_quantity_from_element(vm, elements.next())?; | |||
| let (quantity, _) = try_update_quantity_from_element(vm, elements.next())?; | |||
There was a problem hiding this comment.
this also lends to the fact that try_update_quantity_from_element might be doing two jobs that aren't always needed.
There was a problem hiding this comment.
That is a fair point. I think it could be possible to add a new, separate function that only checks the sign of the given element and set the flag directly, without messing with return types.
I did what I did yesterday mainly because I originally did not know that I could just set left adjustment to spec flags directly, so I wanted to have some means of storing the alignment data to later reference when I'm actually formatting the string. However, I think I forgot to adjust later when I discovered what I can do with CConversionFlags.
Let me work on the fix real quick.
…to separate function
More details can be found here
#4762
It was the last issue that made test_mod to fail, so I also removed the expectedFailure flag from that test.