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

gh-131507: Add support for syntax highlighting in PyREPL by ambv · Pull Request #133247 · python/cpython · GitHub

/ cpython Public

gh-131507: Add support for syntax highlighting in PyREPL - #133247

Merged
ambv merged 23 commits into
python:mainfrom
ambv:pyrepl-syntax-highlighting-tokens
May 2, 2025
ConversationCommits (23)ChecksFiles changed
Merged

gh-131507: Add support for syntax highlighting in PyREPL#133247
ambv merged 23 commits into
python:mainfrom
ambv:pyrepl-syntax-highlighting-tokens

Conversation

ambv commented May 1, 2025 •
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

This is a much improved version of gh-131562. It uses the tokenizer for better speed and pattern matching for more robust handling of soft keywords. While it still won't hit all cases correctly, it's better than idlelib's regular expression-based colorizer and unlike our glorious PEG parser it supports incomplete input, which is crucial for an interactive shell.

Relatedly, pasting support was tweaked to be way faster. Now the entire contents of Frankenstein can be pasted within 3 seconds both on Unix and Windows as long as bracketed pasting is supported by the terminal. This is a necessary tweak for syntax highlighting not to cripple performance of pastes above 5kB.

There is experimental support for theming through _colorize.set_theme() that's mentioned in "What's New" but otherwise undocumented so far.

📚 Documentation preview 📚: https://cpython-previews--133247.org.readthedocs.build/

bedevere-app Bot mentioned this pull request May 1, 2025
Closed
ambv added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section topic-repl Related to the interactive shell labels May 1, 2025

bedevere-bot commented May 1, 2025

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @ambv for commit ffebbbe 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133247%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 1, 2025
tomasr8 reviewed May 1, 2025
View reviewed changes

tomasr8 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

Sorry for being too nitpicky, but I still find the blue used for keywords to be too dark when coupled with dark-themed terminals (e.g. #131562 (comment)). Do you think we could use a more contrasting color?

Comment thread Lib/_pyrepl/utils.py Outdated
Comment thread Lib/_pyrepl/utils.py Outdated

ambv commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

I disagree about the blue color, the fact that Ubuntu colors it too dim is not actionable for the Python project. Adjust your terminal.

ambv commented May 1, 2025

Copy link
Copy Markdown
Contributor Author

The refleak failures are unrelated, see #133258.

chris-eibl commented May 2, 2025

Copy link
Copy Markdown
Member

@chris-eibl I simplified the Unix case to remove the additional buffering since it complicates the codebase. Your PR was adding that same buffering to Windows.

Sorry for only spotting the windows_console.py change - I must be biased :)

Yeah, simple is always better 👍

Given that we now achieve the same performance without double-buffering, I'd say we don't need the other PR.

I can confirm: this PR is now way faster pasting in the virtual terminal mode on Windows 🚀

chris-eibl mentioned this pull request May 2, 2025
Closed
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/utils.py
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/utils.py Outdated
Comment thread Lib/_pyrepl/utils.py
Comment thread Lib/_pyrepl/commands.py
done = "\x1b[201~"
data = ""
import time
start = time.time()

pablogsal May 2, 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

Leftover from testing?

ambv May 2, 2025

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

The trace below shows time. I can move the import up.

pablogsal May 2, 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

Then use perf_counter please

Comment thread Lib/_pyrepl/utils.py
Comment thread Lib/_pyrepl/utils.py
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/reader.py
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/unix_console.py
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/windows_console.py
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/utils.py Outdated
pablogsal reviewed May 2, 2025
View reviewed changes
Comment thread Lib/_pyrepl/utils.py
pablogsal approved these changes May 2, 2025
View reviewed changes

pablogsal 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

I left a bunch of questions and nitpicks but overall this looks fantastic.

I've reviewed the tokenizer-related parts of the implementation and I'm comfortable with the current approach. We have already discussed this offline but for everyone else reading: while the soft keyword detection uses heuristics, this is a reasonable compromise as bringing in a more correct solution would require a full parser run, which would be much heavier and slower for this use case and we don't even have the technology now to do partial input in the PEG parser so whatever we do will be in python and much slower.

I've also run some performance benchmarks and things look really good — the impact on responsiveness is minimal, even with syntax highlighting enabled by default. And on Windows seem to be super fast.

I'm happy to explore further optimizations in the future, such as avoiding repeated tokenization during screen refreshes. But as it stands, this looks solid. Great work!

pablogsal commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

The only open question is what to do for setting the theme officially (right now is a "experimental" API) but I think since that affects other parts of the interpreter (such as tracebacks) this is out of scope of this particular PR, so I propose to discuss this separately

chris-eibl commented May 2, 2025

Copy link
Copy Markdown
Member

In a legacy Windows console, the prompt is no longer colored, but e.g. a SyntaxError still is.

Also, syntax highlighting is turned off (this maybe is wanted behaviour?)

chris-eibl commented May 2, 2025

Copy link
Copy Markdown
Member

can_colorize is True, but _colorize.theme has empty strings for all values.

ambv commented May 2, 2025

Copy link
Copy Markdown
Contributor Author

Good catch, @chris-eibl. I'll fix it forward in a subsequent PR.

ambv merged commit fac41f5 into python:main May 2, 2025

chris-eibl commented May 2, 2025

Copy link
Copy Markdown
Member

Calling _colorize.set_theme() right after virtual terminal processing is enabled via

cpython/Lib/_pyrepl/windows_console.py

Lines 150 to 155 in fac41f5

SetConsoleMode(
OutHandle,
ENABLE_WRAP_AT_EOL_OUTPUT
| ENABLE_PROCESSED_OUTPUT
| ENABLE_VIRTUAL_TERMINAL_PROCESSING,
)

does fix it for me.

ambv deleted the pyrepl-syntax-highlighting-tokens branch May 3, 2025 09:07
This was referenced May 4, 2025
Merged
Closed
hugovk mentioned this pull request May 5, 2025
Closed
stonebig mentioned this pull request May 26, 2025
Closed
tomasr8 mentioned this pull request Jun 3, 2025
Open
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
pythongh-131507: Add support for syntax highlighting in PyREPL (pytho…
be13396 
…nGH-133247)

Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
breindt4 mentioned this pull request Sep 24, 2025
Closed
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

hugovk hugovk left review comments
tomasr8 tomasr8 left review comments
pablogsal pablogsal approved these changes
lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
+1 more reviewer
Viicos Viicos left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

topic-repl Related to the interactive shell

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL