gh-144207: Syntax highlighting(theming support) for dis module #144208
Conversation
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.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
|
All commit authors signed the Contributor License Agreement. |
StanFromIreland
left a comment
StanFromIreland
left a comment
There was a problem hiding this comment.
Please add a NEWS entry and update What's New.
There was a problem hiding this comment.
Please add tests for the colouration.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| False) | ||
|
|
||
| def print_instruction_line(self, instr, mark_as_current): | ||
| def print_instruction_line(self, instr, mark_as_current) -> None: |
There was a problem hiding this comment.
Please revert adding types.
| fail_info: str = ANSIColors.BOLD_RED | ||
| reset: str = ANSIColors.RESET | ||
|
|
||
|
|
There was a problem hiding this comment.
Please re-add this line that was accidentally removed.
| "GET_AWAITABLE", | ||
| "GET_AITER", | ||
| "GET_ANEXT", | ||
| "END_ASYNC_FOR", |
There was a problem hiding this comment.
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.
| exception_label: str = ANSIColors.CYAN | ||
| argument_detail: str = ANSIColors.GREY | ||
|
|
||
| op_stack: str = ANSIColors.BOLD_YELLOW |
There was a problem hiding this comment.
How were those categories determined? are they determined already like that in dis.rst?
| end = entry.end_label | ||
| target = entry.target_label | ||
| print(f" L{start} to L{end} -> L{target} [{entry.depth}]{lasti}", file=file) | ||
| print(f" {theme.exception_label}L{start}{theme.reset} to {theme.exception_label}L{end}{theme.reset} -> {theme.exception_label}L{target}{theme.reset} [{entry.depth}]{lasti}", file=file) |
There was a problem hiding this comment.
Please wrap long lines under 80 chars. Ideally you put the newlines just after defining the color.
Or make temporary variables.
|
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 phrase |
Not to worry, it can stay on by default because colour is automatically disabled in pipes: 
|
|
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) |
|
json, yes: https://docs.python.org/3/whatsnew/3.14.html#json. symtable, no[t yet]. |
4 participants
About
Implemented in similar fashion (e.g unittest, difflib etc.) by defining
ThemeSectionand registered theme onThemeclass.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 via
color_by_opnamemethod.I have trouble with tests, now
disis 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_colorflag(keyword only) todis.disfunction 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 mode
In light mode
No color
Thanks.
dismodule #144207