Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ambv merged 23 commits intopython:mainfromambv:pyrepl-syntax-highlighting-tokens
May 2, 2025

Conversation

ambv
Copy link
Contributor

@ambvambv commentedMay 1, 2025•
edited by github-actionsbot
Loading

This is a much improved version ofgh-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 thanidlelib'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 beway 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/

ZeroIntensity, chris-eibl, and loureq177 reacted with rocket emoji
@ambvambv added 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section topic-replRelated to the interactive shell labelsMay 1, 2025
@bedevere-bot
Copy link

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

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-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 1, 2025
Copy link
Member

@tomasr8tomasr8 left a comment

Choose a reason for hiding this comment

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

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?

@ambv
Copy link
ContributorAuthor

ambv commentedMay 1, 2025

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
Copy link
ContributorAuthor

ambv commentedMay 1, 2025

The refleak failures are unrelated, see#133258.

@skirpichev
Copy link
Member

Sorry for being too nitpicky, but I still find the blue used for keywords to be too dark when coupled with dark-themed terminals

I think that the default theme polishing can be addressed in following pr(s?), as per pr description:

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

@ambv
Copy link
ContributorAuthor

ambv commentedMay 2, 2025

So this passed all buildbots save for s390x. The s390x failure is unrelated, Eric is dealing with it:#133265.

ambvand others added3 commitsMay 2, 2025 14:48
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@chris-eibl
Copy link
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 thewindows_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 🚀

ambv reacted with thumbs up emojitomasr8 reacted with rocket emoji

done = "\x1b[201~"
data = ""
import time
start = time.time()
Copy link
Member

Choose a reason for hiding this comment

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

Leftover from testing?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

pablogsal reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Then useperf_counter please

Copy link
Member

@pablogsalpablogsal left a comment

Choose a reason for hiding this comment

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

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!

chris-eibl and hugovk reacted with hooray emoji
@pablogsal
Copy link
Member

pablogsal commentedMay 2, 2025•
edited
Loading

The only open question is what to do for setting the themeofficially (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
Copy link
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?)

image

@chris-eibl
Copy link
Member

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

@ambv
Copy link
ContributorAuthor

ambv commentedMay 2, 2025

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

@ambvambv merged commitfac41f5 intopython:mainMay 2, 2025
48 checks passed
@chris-eibl
Copy link
Member

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

SetConsoleMode(
OutHandle,
ENABLE_WRAP_AT_EOL_OUTPUT
|ENABLE_PROCESSED_OUTPUT
|ENABLE_VIRTUAL_TERMINAL_PROCESSING,
)

does fix it for me.

ambv reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@hugovkhugovkhugovk left review comments

@tomasr8tomasr8tomasr8 left review comments

@ViicosViicosViicos left review comments

@pablogsalpablogsalpablogsal approved these changes

@lysnikolaoulysnikolaouAwaiting requested review from lysnikolaoulysnikolaou is a code owner

Assignees
No one assigned
Labels
topic-replRelated to the interactive shell
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

8 participants
@ambv@bedevere-bot@chris-eibl@skirpichev@pablogsal@hugovk@tomasr8@Viicos
[8]ページ先頭
©2009-2025 Movatter.jp