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

gh-135148: Correctly handle f/t strings with comments and debug expressions by pablogsal · Pull Request #135198 · python/cpython · GitHub

/ cpython Public

gh-135148: Correctly handle f/t strings with comments and debug expressions - #135198

Merged
pablogsal merged 4 commits into
python:mainfrom
pablogsal:gh-135148
Jul 16, 2025
ConversationCommits (4)ChecksFiles changed
Merged

gh-135148: Correctly handle f/t strings with comments and debug expressions#135198
pablogsal merged 4 commits into
python:mainfrom
pablogsal:gh-135148

Conversation

pablogsal commented Jun 6, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

pablogsal requested a review from lysnikolaou as a code owner June 6, 2025 01:18
bedevere-app Bot mentioned this pull request Jun 6, 2025
Closed
pablogsal force-pushed the gh-135148 branch 2 times, most recently from bd49471 to 5f8456e Compare June 6, 2025 01:22
pablogsal commented Jun 6, 2025
View reviewed changes
Comment thread Parser/lexer/lexer.c Outdated
in_string = 0; // Whether we're currently inside a string
string_quote = 0; // The quote character for current string (' or ")

// Process each character of input

pablogsal Jun 6, 2025

Copy link
Copy Markdown
Member 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

@lysnikolaou please read this carefully. I am mostly sure its correct but it really needs some extra eyes

Wulian233 mentioned this pull request Jun 6, 2025
Merged
lysnikolaou requested changes Jun 6, 2025
View reviewed changes

lysnikolaou left a comment

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

Yeah, I was thinking something very similar as a solution, too! I found one problem, but it looks good otherwise.

Comment thread Parser/lexer/lexer.c Outdated

bedevere-app Bot commented Jun 6, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

lysnikolaou reviewed Jun 9, 2025
View reviewed changes

lysnikolaou left a comment •
edited
Loading

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 looks good now! Thanks @pablogsal!

I left a couple of inline comments for some very minor improvements.

Comment thread Parser/lexer/lexer.c
Comment on lines +141 to +147
if (!in_string) {
in_string = 1;
quote_char = ch;
}
else if (ch == quote_char) {
in_string = 0;
}

lysnikolaou Jun 9, 2025

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 only works because there's always an odd number of quotes in a STRING token and we're sure that all STRING tokens will be valid at this point. Maybe add a comment to explain that?

Comment thread Parser/lexer/lexer.c Outdated
Comment on lines +127 to +128
char quote_char = 0;
char string_quote = 0;

lysnikolaou Jun 9, 2025

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

Do we need both of these?

lysnikolaou commented Jul 13, 2025

Copy link
Copy Markdown
Member

@pablogsal Ping here. Will we be able to get thins into 3.13 before the rc release? If you don't have time to work on it, I'm okay taking it over.

pablogsal commented Jul 13, 2025

Copy link
Copy Markdown
Member Author

I am travelling today :( If you want and have the time you can push the fixes to the PR and land it but if you don't have time maybe I can get to it at night.

lysnikolaou approved these changes Jul 16, 2025
View reviewed changes

lysnikolaou left a comment

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

Pushed a commit with a couple of additions. LGTM now! Let's merge it.

pablogsal merged commit ef66fb5 into python:main Jul 16, 2025
44 of 45 checks passed
pablogsal deleted the gh-135148 branch July 16, 2025 09:47

lysnikolaou commented Jul 16, 2025

Copy link
Copy Markdown
Member

Are we backporting this?

pablogsal commented Jul 16, 2025

Copy link
Copy Markdown
Member Author

We should indeed

pablogsal added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jul 16, 2025

miss-islington-app Bot commented Jul 16, 2025

Copy link
Copy Markdown

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app Bot commented Jul 16, 2025

Copy link
Copy Markdown

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington-app Bot commented Jul 16, 2025

Copy link
Copy Markdown

Sorry, @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker ef66fb597ba909ead2fbfc06f748aa7b7e9ea437 3.13

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 16, 2025
pythongh-135148: Correctly handle f/t strings with comments and debug…
c08f2ab 
… expressions (pythonGH-135198)

(cherry picked from commit ef66fb5)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bedevere-app Bot commented Jul 16, 2025

Copy link
Copy Markdown

GH-136720 is a backport of this pull request to the 3.14 branch.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label Jul 16, 2025
pablogsal pushed a commit that referenced this pull request Jul 16, 2025
[3.14] gh-135148: Correctly handle f/t strings with comments and debu…
9bf3464 
…g expressions (GH-135198) (#136720)

bedevere-app Bot commented Jul 21, 2025

Copy link
Copy Markdown

GH-136899 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jul 21, 2025
lysnikolaou added a commit that referenced this pull request Jul 21, 2025
[3.13] gh-135148: Correctly handle f/t strings with comments and debu…
c1085a0 
…g expressions (#135198) (#136899)

Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
pythongh-135148: Correctly handle f/t strings with comments and debug…
941dba0 
… expressions (python#135198)
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
pythongh-135148: Correctly handle f/t strings with comments and debug…
a2fad4c 
… expressions (python#135198)
kumaraditya303 pushed a commit to miss-islington/cpython that referenced this pull request Sep 9, 2025
[3.14] pythongh-135148: Correctly handle f/t strings with comments an…
f671b96 
…d debug expressions (pythonGH-135198) (python#136720)
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lysnikolaou lysnikolaou approved these changes

Assignees

pablogsal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL