Skip to content

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.

Sign up for GitHub

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

Merged
youknowone merged 5 commits into from
Mar 26, 2023
Merged

Allow try_update_quantity_from_tuple to set left alignment when needed #4766

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84d91ab
Fix https://github.com/RustPython/RustPython/issues/4762
MegasKomnenos Mar 25, 2023
88bb322
Remove expectedFailure flag from test_mod
MegasKomnenos Mar 25, 2023
653cf2e
Fix styling issues
MegasKomnenos Mar 25, 2023
c3881dd
Separate left-adjustment logic from try_update_quantity_from_element …
MegasKomnenos Mar 26, 2023
b7bb7a1
Implement styling feedback
MegasKomnenos Mar 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Lib/test/test_bytes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,8 +704,6 @@ def test_rindex(self):
self.assertEqual(b.rindex(i, 3, 9), 7)
self.assertRaises(ValueError, b.rindex, w, 1, 3)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_mod(self):
b = self.type2test(b'hello, %b!')
orig = b
Expand Down
6 changes: 6 additions & 0 deletions common/src/cformat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,12 @@ impl CFormatString {
}
}

#[derive(Debug, Copy, Clone, PartialEq)]

@DimitrisJim DimitrisJim Mar 25, 2023

Copy link
Copy Markdown
Member

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.

pub enum CFormatAlign {
Left,
Right,
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
34 changes: 27 additions & 7 deletions vm/src/cformat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,18 @@ fn spec_format_string(
fn try_update_quantity_from_element(

@youknowone youknowone Mar 25, 2023

Copy link
Copy Markdown
Member

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:

Suggested change
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(

@MegasKomnenos MegasKomnenos Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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.

vm: &VirtualMachine,
element: Option<&PyObjectRef>,
) -> PyResult<CFormatQuantity> {
) -> PyResult<(CFormatQuantity, CFormatAlign)> {
match element {
Some(width_obj) => {
if let Some(i) = width_obj.payload::<PyInt>() {
let i = i.try_to_primitive::<i32>(vm)?.unsigned_abs();
Ok(CFormatQuantity::Amount(i as usize))
let i = i.try_to_primitive::<i32>(vm)?;
let align = if i < 0 {
CFormatAlign::Left
} else {
CFormatAlign::Right
};
let i = i.unsigned_abs();
Ok((CFormatQuantity::Amount(i as usize), align))
} else {
Err(vm.new_type_error("* wants int".to_owned()))
}
Expand All @@ -222,12 +228,16 @@ fn try_update_quantity_from_tuple<'a, I: Iterator<Item = &'a PyObjectRef>>(
vm: &VirtualMachine,
elements: &mut I,
q: &mut Option<CFormatQuantity>,
f: &mut CConversionFlags,
) -> PyResult<()> {
let Some(CFormatQuantity::FromValuesTuple) = q else {
return Ok(());
};
let quantity = try_update_quantity_from_element(vm, elements.next())?;
let (quantity, align) = try_update_quantity_from_element(vm, elements.next())?;
*q = Some(quantity);
if let CFormatAlign::Left = align {
f.insert(CConversionFlags::LEFT_ADJUST);
}
Ok(())
}

Expand All @@ -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())?;

@DimitrisJim DimitrisJim Mar 25, 2023

Copy link
Copy Markdown
Member

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.

@MegasKomnenos MegasKomnenos Mar 26, 2023

Copy link
Copy Markdown
Contributor Author

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.

*p = Some(CFormatPrecision::Quantity(quantity));
Ok(())
}
Expand Down Expand Up @@ -322,7 +332,12 @@ pub(crate) fn cformat_bytes(
match part {
CFormatPart::Literal(literal) => result.append(literal),
CFormatPart::Spec(spec) => {
try_update_quantity_from_tuple(vm, &mut value_iter, &mut spec.min_field_width)?;
try_update_quantity_from_tuple(
vm,
&mut value_iter,
&mut spec.min_field_width,
&mut spec.flags,
)?;
try_update_precision_from_tuple(vm, &mut value_iter, &mut spec.precision)?;

let value = match value_iter.next() {
Expand Down Expand Up @@ -416,7 +431,12 @@ pub(crate) fn cformat_string(
match part {
CFormatPart::Literal(literal) => result.push_str(literal),
CFormatPart::Spec(spec) => {
try_update_quantity_from_tuple(vm, &mut value_iter, &mut spec.min_field_width)?;
try_update_quantity_from_tuple(
vm,
&mut value_iter,
&mut spec.min_field_width,
&mut spec.flags,
)?;
try_update_precision_from_tuple(vm, &mut value_iter, &mut spec.precision)?;

let value = match value_iter.next() {
Expand Down