-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
2007961
to
410f82f
Compare
There was a problem hiding this 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.
tests/snippets/strings.py
Outdated
assert f'{-10.0:.8%}' == '-1000.00000000%' | ||
assert '{:%}'.format(float('nan')) == 'nan%' | ||
assert '{:%}'.format(float('NaN')) == 'nan%' | ||
assert '{:%}'.format(float('NAN')) == 'nan%' |
There was a problem hiding this comment.
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 nan
s.
There was a problem hiding this comment.
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 👍
vm/src/obj/objfloat.rs
Outdated
let format_spec = FormatSpec::parse(spec.as_str())?; | ||
format_spec.format_float(self.value) | ||
}; | ||
match try_format() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Contributes to RustPython#1656
410f82f
to
765089d
Compare
Thanks for contributing! |
Contributes to #1656