Skip to content

Feature: str.format accepting !r, !s and !a #1027

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 4 commits into from
Jun 10, 2019

Conversation

alanjds
Copy link
Contributor

@alanjds alanjds commented Jun 9, 2019

From: https://gitter.im/rustpython/Lobby?at=5cfa898ebf4cbd167c484cf0

As the Python docs, when a {!r} occur, it runs __repr__ over the argument before applying the : rules. The current implementation of {!a} is not correct, as simple acts as !r.

alanjds added 3 commits June 9, 2019 13:46
By the Python docs, the `!` forces a conversion of the argument before
applying the normal formating logic described after the `:` marker.

See: https://docs.python.org/3.4/library/string.html#format-string-syntax
@alanjds
Copy link
Contributor Author

alanjds commented Jun 9, 2019

@palaviv: Can you please take a look if it suits your needs?

@codecov-io
Copy link

Codecov Report

Merging #1027 into master will increase coverage by 0.03%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   64.71%   64.74%   +0.03%     
==========================================
  Files          97       97              
  Lines       17047    17093      +46     
  Branches     3800     3811      +11     
==========================================
+ Hits        11032    11067      +35     
- Misses       3441     3443       +2     
- Partials     2574     2583       +9
Impacted Files Coverage Δ
vm/src/obj/objstr.rs 73.67% <100%> (-0.34%) ⬇️
vm/src/format.rs 70.29% <85.36%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68011df...854bacf. Read the comment docs.


match chars.next() {
None => None, // Should fail instead?
Some(c) => FormatPreconversor::from_char(c),
Copy link
Member

Choose a reason for hiding this comment

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

This can just be chars.next().map(FormatPreconversor::from_char)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum... this still returns None if chars.next() is None?

Asking because FormatPreconversor::from_char is not prepared to handle None as input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another aspect: // Should fail instead?

I do not know how to raise exceptions. Ideally, {!x} should fail, but this code will pass for now:

# CPy 3.6
>>> '{!x}'.format(123)
Traceback (most recent call last):
  File "<ipython-input-2-b294f18f6498>", line 1, in <module>
    '{!x}'.format(123)
ValueError: Unknown conversion specifier x
# RustPython
>>>>> '{!x}'.format(123)
'123'

Copy link
Contributor

@silmeth silmeth Jun 11, 2019

Choose a reason for hiding this comment

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

next().map(FormatPreconversor::from_char) would just map the inner value of the Option if present – would do exactly what you did (pass the inner value to from_char() if it exists, or return None if the Option is None).

To raise an exception, you’d need to signal an error somehow (eg. return a Result<Option<FormatPreconversor>, String> instead of an Option<FormatPreconversor>) and then check for the result in call_object_format() and in case of an error, return a new vm error there (eg. by return Err(vm.new_value_error(…))). Or just by mapping the error:

let (preconversor, new_format_spec) = FormatPreconversor::parse_and_consume(format_spec);
let preconversor = preconversor.map_err(|msg| vm.new_value_error(msg))?;
// …

But then you’d probably want to return Result<(Option<FormatPreconversor>, &str), String> rather than (Result<Option<FormatPreconversor>, String>, &str), so that it’d look like:

let (preconversor, new_format_spec) = FormatPreconversor::parse_and_consume(format_spec)
    .map_err(|msg| vm.new_value_error(msg))?;

See https://doc.rust-lang.org/std/result/

@palaviv
Copy link
Contributor

palaviv commented Jun 10, 2019

This looks great to me. Thanks @alanjds this will help me very much in #1018.

@coolreader18 coolreader18 merged commit 740e838 into RustPython:master Jun 10, 2019
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.

5 participants