Skip to content

Add support for the % format code for floats. #1657

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 1 commit into from
Jan 4, 2020

Conversation

doyshinda
Copy link
Contributor

Contributes to #1656

@doyshinda doyshinda force-pushed the impl_percent_format branch from 2007961 to 410f82f Compare January 3, 2020 04:09
Copy link
Member

@coolreader18 coolreader18 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! I made a few comments and once those are addressed this is ready to merge.

assert f'{-10.0:.8%}' == '-1000.00000000%'
assert '{:%}'.format(float('nan')) == 'nan%'
assert '{:%}'.format(float('NaN')) == 'nan%'
assert '{:%}'.format(float('NAN')) == 'nan%'
Copy link
Member

Choose a reason for hiding this comment

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

The difference in casing is kinda redundant, as the formatting machinery isn't really involved; it's float.__new__ that parses the nans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is, will remove 👍

let format_spec = FormatSpec::parse(spec.as_str())?;
format_spec.format_float(self.value)
};
match try_format() {
Copy link
Member

Choose a reason for hiding this comment

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

You could do something like

match FormatSpec::parse(spec.as_str()).and_then(|spec| spec.format_float(self.value)) {
    ...
}

to avoid the closure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Out of curiosity, what are the benefits (if any) of doing as shown above vs. using a closure? Or is it just Rust convention?

Copy link
Member

Choose a reason for hiding this comment

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

Since it's not a very complicated operation, it's simpler to use the combinator method than define a closure and immediately call it once. It's similar to do vs >>= if you've worked with Haskell (do notation considered harmful).

let (format_type, after_format_type) = parse_format_type(after_precision);
if !after_format_type.is_empty() {
return Err("Invalid format spec");
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add some tests that ensure that this properly fails with characters left after the format type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@doyshinda doyshinda force-pushed the impl_percent_format branch from 410f82f to 765089d Compare January 3, 2020 23:14
@coolreader18
Copy link
Member

Thanks for contributing!

@coolreader18 coolreader18 merged commit b3d887f into RustPython:master Jan 4, 2020
@doyshinda doyshinda deleted the impl_percent_format branch January 4, 2020 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants