gh-149026: Add colour to pickletools CLI output#149027
gh-149026: Add colour to pickletools CLI output#149027hugovk wants to merge 4 commits intopython:mainfrom
pickletools CLI output#149027Conversation
|
Aside: the alignment is the same in # make a mild effort to align arguments
...
# make a mild effort to align annotations
...If someone's up for making a bit less mild effort in another PR, go for it! |
|
Can we have the instruction names in white (no color)? I find it less readable with colors as all instructions have the same color. |
|
IMO, too many colors hurt more than they help. White is not that good either actually... I do like the colors for literals and the first column but I feel that colors for instruction names are always too much because it's just adding colors to words that will anyway be classified identically. Alternatively, how about some "dead" words such as MARK/NONE etc and add categorize the opcodes themselves into colorized categories. This could help a bit more visually (e.g., I want to locate all MARK, whether I use colors or not now I won't be able to find it easily without a CTRL+F). |
|
Much better IMO! I would suggest not having colors for opcodes that have a colored literal (like SHORT_BINUNICODE). WDYT? |
It would require a bit more effort to parse the literal in this case. I think the no-color option is better IMO. FTR, the MARK can be in gray as the PROTO because each MARK is followed by an indent I thnk. |
|
I like those colors, at least they are more readable to me (and I have very poor eyesight). What about light theme though? |
picnixz
left a comment
There was a problem hiding this comment.
I find the colors much readable now! thanks for the work
| indentchunk = ' ' * indentlevel | ||
| errormsg = None | ||
| annocol = annotate # column hint for annotations | ||
| t = get_theme(tty_file=out if out is not None else sys.stdout).pickletools |
There was a problem hiding this comment.
For follow up:
- Allow tty_file to be None and if so, fallback to sys.stdout?
- Is it always safe to use the "t" variable? I love using variables with 1 letter in general but since we don't run linters, we may not always catch shadowing if it happens so maybe we should quickly recheck that in the other existing parts of the stdlib.
There was a problem hiding this comment.
- Allow tty_file to be None and if so, fallback to sys.stdout?
Yes, it can be None, and ends up at this fallback:
Lines 520 to 521 in 62792c8
- Is it always safe to use the "t" variable? I love using variables with 1 letter in general but since we don't run linters, we may not always catch shadowing if it happens so maybe we should quickly recheck that in the other existing parts of the stdlib.
I'm generally not a fan of one-letter variables :) But I think it's okay here to make the f-strings more readable, and we've been doing that since adding this theme support: #133347.
There was a problem hiding this comment.
Hum, then we should just pass out directly, insteada of
tty_file=out if out is not None else sys.stdout (or am I missing something?
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>







Create a pickle file, for example:
Then
./python.exe -m pickletools data.pickle:pickletoolsCLI output #149026📚 Documentation preview 📚: https://cpython-previews--149027.org.readthedocs.build/