Skip to content

Add basic number formatting #4461

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Add basic number formatting #4461

wants to merge 3 commits into from

Conversation

notJoon
Copy link
Contributor

@notJoon notJoon commented Jan 19, 2023 •
edited
Loading

Description

Only basic functionalities have been implemented so that formatting can be applied and output even when a 'float' type number comes in.

However, not all functions are fully functional(i.e, decimal rounding or inf value handling.), so it seems that additional functions will need to be supplemented in the future.

Example

## before
print('{:5n}'.format(1234567.1))
>>> Traceback (most recent call last):
    File "<wasm>", line 1, in <module>
    ValueError: Format code 'n' for object of type 'float' not implemented yet

## after
>>> 1.23457e+06
a = '{:.8n}'.format(12345.12345)

type(a)
>>> <class 'str'>

print(a)
>>> 12345.123

Related Issue

#1656

fanninpm
fanninpm reviewed Jan 19, 2023
View reviewed changes
Copy link
Contributor

@fanninpm fanninpm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nitpicks regarding the usage of match expressions vs. if expressions. If you decide to apply these suggestions, please remember to run them through rustfmt.

@notJoon
Copy link
Contributor Author

notJoon commented Jan 21, 2023

@fanninpm Thanks for your advice! It's definitely better than before 👍👍

DimitrisJim
DimitrisJim reviewed Jan 21, 2023
View reviewed changes
@youknowone
Copy link
Member

The newly added test seems fail in CPython 3.10

Traceback (most recent call last):
  File "/home/runner/work/RustPython/RustPython/extra_tests/snippets/builtin_str.py", line 619, in <module>
    assert '{:5n}'.format(1e+3) == " 1000"
AssertionError

youknowone
youknowone requested changes Jan 22, 2023
View reviewed changes
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.

test.test_format.FormatTest is now unexpected success because you correctly fixed it!
We now can remove @unittest.expectedFailure from the test.

@notJoon
Copy link
Contributor Author

notJoon commented Jan 22, 2023 •
edited
Loading

@youknowone I don't know if this is right way, but I removed @unittest.expectedFailure from the test.test_format.FormatTest.

@DimitrisJim
Copy link
Member

DimitrisJim commented Jan 22, 2023 •
edited
Loading

@notJoon it's the right way! You should also remove the # TODO comments before the decorators though since they aren't needed anymore.

Clippy seems to be complaining about the usage of the WhiteSpace variant, there should be a call here where we supply WhiteSpace as the padding though (or else there wouldn't be much point to an enum)

DimitrisJim
DimitrisJim reviewed Jan 22, 2023
View reviewed changes
@notJoon notJoon requested a review from youknowone January 30, 2023 09:38
youknowone
youknowone requested changes Feb 9, 2023
View reviewed changes
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.

Please resolve errors of Linux CI run
https://github.com/RustPython/RustPython/actions/runs/3980709695/jobs/6823974987

@youknowone youknowone added C-compat A discrepancy between RustPython and CPython A-stdlib labels Feb 22, 2023
@fanninpm fanninpm mentioned this pull request Mar 3, 2023
Merged
youknowone
youknowone reviewed Mar 4, 2023
View reviewed changes
common/src/format.rs
@@ -384,14 +404,20 @@ impl FormatSpec {

fn get_separator_interval(&self) -> usize {
match self.format_type {
Some(FormatType::Number) => 9999,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meaning something or just intended a big number? I think bigint can have more digits than 9999.

@youknowone
Copy link
Member

I thought this is done except for test decorators but seems still on working, right?

youknowone
youknowone reviewed Mar 4, 2023
View reviewed changes
common/src/format.rs
@@ -312,13 +334,15 @@ impl FormatSpec {
inter: i32,
sep: char,
disp_digit_cnt: i32,
padding: PaddingStyle,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this padding. It only allows whitespace or 0, but whitespace padding doesn't add seprator.

@notJoon notJoon closed this by deleting the head repository Mar 21, 2024
@youknowone youknowone mentioned this pull request Jul 8, 2024
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowone youknowone youknowone requested changes

@DimitrisJim DimitrisJim DimitrisJim left review comments

@fanninpm fanninpm fanninpm left review comments

Assignees
No one assigned
Labels
A-stdlib C-compat A discrepancy between RustPython and CPython
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@notJoon @youknowone @DimitrisJim @fanninpm