-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow try_update_quantity_from_tuple to set left alignment when needed #4766
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style suggestion for immutable arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thank you!
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.