8000 Allow try_update_quantity_from_tuple to set left alignment when needed by MegasKomnenos · Pull Request #4766 · RustPython/RustPython · GitHub
[go: up one dir, main page]

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.

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
merged 5 commits into from
Mar 26, 2023

Conversation

MegasKomnenos
Copy link
Contributor
@MegasKomnenos MegasKomnenos commented Mar 25, 2023

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.

@MegasKomnenos MegasKomnenos changed the title Fix issue https://github.com/RustPython/RustPython/issues/4762 Allow try_update_quantity_from_tuple to set left alignment when needed Mar 25, 2023
@@ -204,12 +204,18 @@ fn spec_format_string(
fn try_update_quantity_from_element(
Copy link
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(

Copy link
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.

@youknowone youknowone requested a review from DimitrisJim March 25, 2023 16:02
@@ -754,6 +754,12 @@ impl CFormatString {
}
}

#[derive(Debug, Copy, Clone, PartialEq)]
Copy link
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.

@@ -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())?;
Copy link
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.

Copy link
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.

Copy link
Member
@youknowone youknowone left a 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

@youknowone youknowone added A-stdlib z-ca-2023 Tag to track contrubution-academy 2023 labels Mar 26, 2023
Copy link
Member
@youknowone youknowone left a 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!

@youknowone youknowone merged commit 1795740 into RustPython:main Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stdlib z-ca-2023 Tag to track contrubution-academy 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0