Skip to content

gh-133346: Make theming support in _colorize extensible - #133347

Merged
ambv merged 21 commits into
python:mainfrom
ambv:colorize-theme-support
May 5, 2025
Merged

gh-133346: Make theming support in _colorize extensible#133347
ambv merged 21 commits into
python:mainfrom
ambv:colorize-theme-support

Conversation

@ambv

@ambvambv commented May 3, 2025

Copy link
Copy Markdown
Contributor

See issue for design details.

Comment threadLib/_pyrepl/utils.py
class ColorSpan(NamedTuple):
span: Span
tag: _colorize.ColorTag
tag: str

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm a little sad about this but currently Python typing cannot construct a type that is "a set of string literals from this type's attributes" and I didn't feel like repeating myself in _colorize.

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.

:(

Comment threadLib/_pyrepl/reader.py Outdated
Comment threadLib/_colorize.py Outdated
Comment threadLib/_colorize.py Outdated
Comment threadLib/_colorize.py Outdated
Comment threadLib/_colorize.py Outdated
ambvand others added 2 commits May 3, 2025 19:26
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Comment threadLib/_colorize.py

@hugovkhugovk 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.

Thanks, it's much nicer to specify semantic meanings rather than colours in the code.

Comment threadLib/_colorize.py Outdated
Comment threadLib/_colorize.py Outdated
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@ambv
ambv requested a review from gaogaotiantian as a code ownerMay 5, 2025 09:24
@hugovkhugovk mentioned this pull request May 5, 2025
Comment threadLib/_colorize.py
Comment threadLib/_colorize.py
Comment threadLib/_colorize.py
Comment threadLib/_colorize.py
Comment threadLib/_pyrepl/utils.py
Comment threadLib/json/tool.py
Comment threadLib/test/test_pdb.py
Comment threadLib/test/test_traceback.py

@pablogsalpablogsal 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.

This LGTM I left some comments and questions but nothing blocking.

I have a request though: I think we should explore offering some semi-final non-experimental way to support this in 3.14 with proper documentation. If @hugovk is fine with this, I would like to propose a discussion in an issue and maybe with the SC. I don't particularly thing this needs a PEP by any means (my personal opinion non SC hat on) but I think that releasing 3.14 with colorize.set_theme() with the docs being a comment is not great and maybe it is worth an exception.

Comment threadLib/_colorize.py
Comment threadLib/_colorize.py Outdated
Comment threadLib/test/test_traceback.py
@hugovk

Copy link
Copy Markdown
Member

I have a request though: I think we should explore offering some semi-final non-experimental way to support this in 3.14 with proper documentation. If @hugovk is fine with this, I would like to propose a discussion in an issue and maybe with the SC. I don't particularly thing this needs a PEP by any means (my personal opinion non SC hat on) but I think that releasing 3.14 with colorize.set_theme() with the docs being a comment is not great and maybe it is worth an exception.

Fine by me.

@tomasr8tomasr8 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.

Exciting stuff! Json changes look good 🙂

Comment threadLib/test/test_json/test_tool.py
@ambv
ambv merged commit f610bbd into python:mainMay 5, 2025
Comment threadLib/_colorize.py
import sys

from collections.abc import Callable, Iterator, Mapping
from dataclasses import dataclass, field, Field

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a little unfortunate for import times that this will bring a dependency on dataclasses module to all modules that import this. :-(

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
…GH-133347)
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ambv@bedevere-bot@hugovk@tomasr8@danielhollas@pablogsal