8000 gh-133346: Make theming support in _colorize extensible by ambv · Pull Request #133347 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
May 5, 2025

Conversation

ambv
Copy link
Contributor
@ambv ambv commented May 3, 2025

See issue for design details.

@@ -44,7 +49,7 @@ def from_token(cls, token: TI, line_len: list[int]) -> Self:

class ColorSpan(NamedTuple):
span: Span
tag: _colorize.ColorTag
tag: str
Copy link
Contributor Author

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
Member

Choose a reason for hiding this comment

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

:(

ambv and others added 2 commits May 3, 2025 19:26
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@ambv ambv requested a review from savannahostrowski as a code owner May 4, 2025 18:30
Copy link
Member
@hugovk hugovk left a comment

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.

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 4, 2025
@ambv ambv requested a review from gaogaotiantian as a code owner May 5, 2025 09:24
@hugovk hugovk mentioned this pull request May 5, 2025
Copy link
Member
@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Hide comment

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.

@hugovk
Copy link
Member
hugovk commented May 5, 2025

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.

Copy link
Member
@tomasr8 tomasr8 left a comment

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 🙂

@ambv ambv merged commit f610bbd into python:main May 5, 2025
48 checks passed
import io
import os
import sys

from collections.abc import Callable, Iterator, Mapping
from dataclasses import dataclass, field, Field
Copy link
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. :-(

Sign up for free to 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
0