Skip to content

gh-93283: Improve error message for f-string with invalid conversion character #93349

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
Merged
merged 3 commits into from
May 31, 2022
Merged

gh-93283: Improve error message for f-string with invalid conversion character #93349

merged 3 commits into from
May 31, 2022

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 30, 2022 •
edited
Loading

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label May 30, 2022
@serhiy-storchaka serhiy-storchaka linked an issue May 30, 2022 that may be closed by this pull request
Unexpected whitespace handling in f-string !r #93283
Closed
serhiy-storchaka
serhiy-storchaka commented May 30, 2022
View reviewed changes
Copy link
Member Author

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

What error should be for f'{3!g'? "invalid conversion character" or "expecting '}'"?

What error should be for f'{3!:'? "missed conversion character" or "expecting '}'"?

@AA-Turner
Copy link
Member

AA-Turner commented May 30, 2022 •
edited
Loading

Personally, I'd expect expecting '}', and then after I add the closing }, I'd then expect invalid conversion character 'g'.

A

@pablogsal
Copy link
Member

pablogsal commented May 30, 2022 •
edited
Loading

expecting '}' is much better IMHO.

Btw we are just working on this in the "porting f-strings to the grammar" project in case you have time to help @serhiy-storchaka

@serhiy-storchaka
Copy link
Member Author

It is easy for f'{3!g', but for f'{3!:' the conversion character is raised before parsing of the format string is even started.

ericvsmith
ericvsmith reviewed May 30, 2022
View reviewed changes
Lib/test/test_fstring.py
"expected 's', 'r', or 'a'" % conv,
["f'{3!" + conv + "}'"])

self.assertAllRaise(SyntaxError,
Copy link
Member

Choose a reason for hiding this comment

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

Somewhere we should probably add whitespace both before and after the conversion character: "s ", " s", " s ". It looks like that got dropped.

@serhiy-storchaka
Copy link
Member Author

Btw we are just working on this in the "porting f-strings to the grammar" project in case you have time to help @serhiy-storchaka

The idea is good at first glance, I thought about it, but it looks too complicated. The main problem is that the closing quote must match the opening one. It can be solved, but it needs significant effort. There are easier and more important problems.

ericvsmith
ericvsmith approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka serhiy-storchaka merged commit 07df8d5 into python:main May 31, 2022
@serhiy-storchaka serhiy-storchaka deleted the fstring-bad-conv-char branch May 31, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericvsmith ericvsmith ericvsmith approved these changes

@pablogsal pablogsal Awaiting requested review from pablogsal pablogsal is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

Assignees
No one assigned
Labels
type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unexpected whitespace handling in f-string !r
5 participants
@serhiy-storchaka @AA-Turner @pablogsal @ericvsmith @bedevere-bot