Skip to content

Fix typing of RegexHighlighter.highlights using ClassVar#3925

Closed
SudheerKovvuru wants to merge 2 commits intoTextualize:masterfrom
SudheerKovvuru:fix-highlighter-typing
Closed

Fix typing of RegexHighlighter.highlights using ClassVar#3925
SudheerKovvuru wants to merge 2 commits intoTextualize:masterfrom
SudheerKovvuru:fix-highlighter-typing

Conversation

@SudheerKovvuru
Copy link
Copy Markdown
Contributor

Type of changes

  • Bug fix
  • New feature
  • Documentation / docstrings
  • Tests
  • Other

AI?

  • AI was used to generate this PR

AI generated PRs may be accepted, but only if @willmcgugan has responded on an issue or discussion.

Checklist

  • I've run the latest black with default args on new code.
  • I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate (see note about typos above).
  • I've added tests for new code.
  • I accept that @willmcgugan may be pedantic in the code review.

Description

Please describe your changes here.
This PR updates the typing of RegexHighlighter.highlights to explicitly mark it as a class variable using ClassVar and an immutable tuple.

The change is limited to typing and does not alter runtime behavior.

Note: Some rendering-related tests fail locally on Windows (Python 3.11), which appears to be environment-specific and unrelated to this change.

Important: Link to an issue or discussion regarding these changes.
Fixes #3888

Comment thread rich/highlighter.py Outdated
"""Applies highlighting from a list of regular expressions."""

highlights: List[str] = []
highlights: ClassVar[tuple[str | re.Pattern[str], ...]] = ()
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.

But it is a list. And while it does coincidentally work with a pattern, I can think of no good reason to pre-compile it. the re library will cache compilation anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the clarification, that makes sense.
I’ve updated the type to ClassVar[tuple[str, ...]] to better reflect actual usage and avoid implying pre-compilation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!
Since the issue has now been resolved and merged upstream via #3939, I’m closing this PR to avoid duplication.
Appreciate the clarification and feedback.

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.

[BUG] Typing of highlighter needs improvement

2 participants