Repository navigation
Allow try_update_quantity_from_tuple to set left alignment when needed - #4766
Conversation
This test no longer fails
MegasKomnenos
changed the title
| @@ -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.
| } | ||
| } | ||
|
|
||
| #[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.
| 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
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
style suggestion for immutable arguments
3 participants
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.