Skip to content

Conversation

svlandeg
Copy link
Member

By using Default(DEFAULT_MARKUP_MODE), rich_markup_mode would be a typer.models.DefaultPlaceholder object and comparing it to other literal strings like MARKUP_MODE_RICH would fail. This resulted in flaky behaviour such as reported in #976, where Rich formatting worked fine when specifying it correctly, but the default value (with Rich installed) didn't result in the same behaviour.

This PR fixes that by simply not using Default for this value - there's no real reason to use it anyway, as it's unimportant to the application whether it's a default or a user-set value.

@svlandeg svlandeg added the bug Something isn't working label Sep 15, 2025
@svlandeg svlandeg marked this pull request as draft September 15, 2025 12:24
@svlandeg
Copy link
Member Author

The failing coverage test reveals that there are code paths in rich_utils.py that are redundant. They were only taken before by accident, because the comparisons weren't done properly with the Default() object.

In fact, the formatting methods in rich_utils shouldn't be called when rich_markup_mode is None (calling them would be problematic as rich would be imported!). To clarify this in code and to avoid further issues in the future, I've introduced MarkupModeStrict and removed the code paths that are redundant.

@svlandeg svlandeg marked this pull request as ready for review September 15, 2025 12:48
@svlandeg svlandeg requested a review from YuriiMotov September 15, 2025 12:58
Copy link

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

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

LGTM!

@svlandeg svlandeg marked this pull request as draft September 19, 2025 07:55
@svlandeg svlandeg self-assigned this Sep 19, 2025
@svlandeg
Copy link
Member Author

svlandeg commented Sep 19, 2025

Feels to me like the CI is failing due to an unrelated issue - I'll investigate.

[UPDATE]: fixed with #1333

def test_markup_mode_default():
# We're assuming the test suite is run with rich installed
app = typer.Typer()
assert app.rich_markup_mode == "rich"
Copy link
Member Author

Choose a reason for hiding this comment

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

on master, this fails with

<typer.models.DefaultPlaceholder object at 0x00000227BB50B130> != rich

@svlandeg svlandeg marked this pull request as ready for review September 23, 2025 09:23
@svlandeg svlandeg removed their assignment Sep 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants