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-144207: Syntax highlighting(theming support) for dis module#144208

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

Open
AbduazizZiyodov wants to merge14 commits intopython:main
base:main
Choose a base branch
Loading
fromAbduaziz-Forks:dis-theme-support

Conversation

@AbduazizZiyodov
Copy link

@AbduazizZiyodovAbduazizZiyodov commentedJan 24, 2026•
edited
Loading

About

Implemented in similar fashion (e.g unittest, difflib etc.) by definingThemeSection and registered theme onTheme class.

Basic highlighting shown in forum(green for all opcodes), but in this PR: one color maps to group of opcodes (e.g. blue for binary opcodes) -- gives better context and it is determined viacolor_by_opname method.

I have trouble with tests, nowdis is defaulting to colored output and 2 tests were failing. I had to set environment variable:NO_COLOR=1... I think, there might be better ways of doing this, one is to defineforce_no_color flag(keyword only) todis.dis function which I'm not certain. So, I would like to elaborate with you on that.

EDIT: Any ideas/additions on color mapping is encouraged :)

Sample screenshots

In dark modedark_01dark_02
In light modelight_01light_02
No colorno_color_01

Thanks.

Highlights opcode's name, arguments, exception table labels.
…ecause dis is not defaulting to syntax highligthing. Of course it needs better handling which I'm not sure now.
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@python-cla-bot
Copy link

python-cla-botbot commentedJan 24, 2026•
edited
Loading

All commit authors signed the Contributor License Agreement.

CLA signed

Copy link
Member

@StanFromIrelandStanFromIreland left a comment

Choose a reason for hiding this comment

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

Please add a NEWS entry and update What's New.

AbduazizZiyodov reacted with thumbs up emoji

Choose a reason for hiding this comment

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

Please add tests for the colouration.

AbduazizZiyodov reacted with thumbs up emoji
@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

Copy link
Member

@picnixzpicnixz left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Please add a What's News entry and tests.I personally do not like this by default since I use grep/cut on the dis output so I would prefer a CLI option to enable colors. EDIT: not a concern anymore

AbduazizZiyodov reacted with thumbs up emoji
"GET_AWAITABLE",
"GET_AITER",
"GET_ANEXT",
"END_ASYNC_FOR",
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused with END_FOR and END_ASYNC_FOR being colored differently but I do not remember the exact effect of the latter.

Choose a reason for hiding this comment

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

I'veread thatEND_FOR is equivalent(or alias I would say) toPOP_TOP:

Removes the top-of-stack item. Equivalent to POP_TOP. Used to clean up at the end of loops, hence the name.

and it is specified under "General instructions" section whileEND_ASYNC_FOR in "Coroutine opcodes" (I generalized this into "control flow" opcodes).

Almost all opcodes are dealing with stack, butEND_ASYNC_FOR is puttinglittle more effort thanEND_FOR which is just stack.pop(), that's why I thoughtEND_ASYNC_FOR is different thanEND_FOR.

That's my understanding.

We might elaborate our discussion on categories in your next comment too.

exception_label: str = ANSIColors.CYAN
argument_detail: str = ANSIColors.GREY

op_stack: str = ANSIColors.BOLD_YELLOW
Copy link
Member

Choose a reason for hiding this comment

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

How were those categories determined? are they determined already like that in dis.rst?

Choose a reason for hiding this comment

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

are they determined already like that in dis.rst?

Almost, I first grouped according todis.rst then re-categorized them according to my understanding (how these opcodes relate, semantically) -- which might need some refinement too.

@bedevere-app
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phraseI have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hugovk
Copy link
Member

I personally do not like this by default since I use grep/cut on the dis output

Not to worry, it can stay on by default because colour is automatically disabled in pipes:

image

@picnixz
Copy link
Member

Oh! great then. I was worried about this. Do we plan to have colors for JSON? (or maybe we already do?) or for symtable? (the symtable cli is very rudimentary and I sometimes want a better one).

There are few modules who output formatted code (AFAIR, dis, symtable, json or even ast) so I wondered whether those would also be colorized. And if we eventually plan to add colors wherever we can (it should be a separate DPO thread/gh issue)

AbduazizZiyodov reacted with rocket emoji

@hugovk
Copy link
Member

json, yes:https://docs.python.org/3/whatsnew/3.14.html#json. symtable, no[t yet].

@bedevere-app
Copy link

Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply theskip news label instead.

@AbduazizZiyodov
Copy link
Author

Oh! great then. I was worried about this. Do we plan to have colors for JSON? (or maybe we already do?) or for symtable? (the symtable cli is very rudimentary and I sometimes want a better one).

There are few modules who output formatted code (AFAIR, dis, symtable, json or even ast) so I wondered whether those would also be colorized. And if we eventually plan to add colors wherever we can (it should be a separate DPO thread/gh issue)@picnixz

Indeed!symtable needs this enhancement as you mentioned, I'll work on this too after this PR gets merged, if anyone hasn't planned.

Copy link
Member

@StanFromIrelandStanFromIreland left a comment

Choose a reason for hiding this comment

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

Please add tests.

@AbduazizZiyodov
Copy link
Author

I'll focus on tests, and try suggested ideas from discussion forum too (probably reducing number of categories maybe).

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

Reviewers

@sobolevnsobolevnsobolevn left review comments

@StanFromIrelandStanFromIrelandStanFromIreland left review comments

@picnixzpicnixzpicnixz requested changes

@hugovkhugovkAwaiting requested review from hugovkhugovk is a code owner

@AA-TurnerAA-TurnerAwaiting requested review from AA-TurnerAA-Turner is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AbduazizZiyodov@hugovk@picnixz@sobolevn@StanFromIreland
[8]ページ先頭
©2009-2026 Movatter.jp