FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Feature: str.format accepting !r, !s and !a by alanjds · Pull Request #1027 · RustPython/RustPython · GitHub

Repository navigation

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

Merged
coolreader18 merged 4 commits into
RustPython:masterfrom
alanjds:format-bang
Jun 10, 2019
Merged

coolreader18 merged 4 commits into
RustPython:masterfrom
alanjds:format-bang

Conversation

alanjds commented Jun 9, 2019

Copy link
Copy Markdown
Contributor

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 commented Jun 9, 2019

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown

Codecov Report

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

@@            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.

Comment thread vm/src/format.rs

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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'

silmeth Jun 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jun 10, 2019

Copy link
Copy Markdown
Contributor

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

coolreader18 merged commit 740e838 into RustPython:master Jun 10, 2019
alanjds deleted the format-bang branch July 30, 2025 22:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL