Skip to content
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

🐛 Ensure rich_markup_mode=None disables Rich formatting #859

Merged
merged 6 commits into from
Aug 24, 2024

Conversation

svlandeg
Copy link
Member

@svlandeg svlandeg commented Jun 12, 2024

This PR is an alternative to #726.

First, it fixes the bug described in #853, namely that

cli = typer.Typer(rich_markup_mode=None)

does not actually disable Rich formatting on the current master, when Rich is installed. This is because many of the methods in core.py only check whether rich is available before calling the functions in rich_utils.py. Instead, the value of rich_markup_mode should be checked as well, as in in PR #726 and as in this PR.

However, when we do that, we effectively make None the default value, even when Rich is installed. This aligns with the current documentation:

By default, rich_markup_mode is None, which disables any rich text formatting.

However, one could argue that this behaviour is a bit unexpected, and that Rich formatting should be the default when Rich is installed. This is what the test suite currently assumes on master, and attempting to change the test suite (as in #726) shows that there would be many breaking cases for users as well.

So, as an alternative to #726 I suggest to fix the original bug, while keeping the default behaviour as it was: Rich formatting if Rich is installed, no formatting if it isn't installed. By introducing a new var DEFAULT_MARKUP_MODE this behaviour is formalized. This will also pave the way to potentially expending this functionality and allowing the user to set this default globally, as proposed in #647. For now, I would focus first on getting this bug resolved and deciding what the defaults should look like.

If we do decide to merge this PR, we can close #726 (or vice versa).

@svlandeg svlandeg added bug Something isn't working p2 labels Jun 12, 2024
@svlandeg svlandeg marked this pull request as draft June 12, 2024 10:02
@svlandeg svlandeg marked this pull request as ready for review June 12, 2024 10:10
Copy link

📝 Docs preview for commit 01fef55 at: https://bd907d4a.typertiangolo.pages.dev

rounded = ["╭", "─", "┬", "╮", "│", "├", "┼", "┤", "╰", "┴", "╯"]


def test_rich_markup_mode_none():
Copy link
Member Author

Choose a reason for hiding this comment

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

This test currently fails on master, which is clearly a bug IMO.

@JacobKochems
Copy link

JacobKochems commented Jun 12, 2024

So, as an alternative to #726 I suggest to fix the original bug, while keeping the default behaviour as it was: Rich formatting if Rich is installed, no formatting if it isn't installed. By introducing a new var DEFAULT_MARKUP_MODE this behaviour is formalized. This will also pave the way to potentially expending this functionality and allowing the user to set this default globally, as proposed in #647. For now, I would focus first on getting this bug resolved and deciding what the defaults should look like.

I second the idea of keeping the default behaviour as a function of Rich's installation status. I think the easiest way towards the user defined global default is to close #647 and merge this bugfix. After which I would take a shot at reimplementing #647 on top of this fix. I would open a new PR for that and mention #647 there as a reference.

Copy link

@JacobKochems JacobKochems left a comment

Choose a reason for hiding this comment

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

I cloned this branch, reviewed the test

tests/test_rich_markup_mode.py

and ran it locally on my machine. Both test cases pass.
The documentation change looks good to me, too.

@moskupols
Copy link

This would really help in my project. I like having a canonization test for --help of my utilities. Currently such a test has to depend on whether rich is installed, and with rich installed the output is full of unicode characters. I'd prefer enforcing the plain help for this test.

Copy link

github-actions bot commented Aug 5, 2024

📝 Docs preview for commit 16ce4e5 at: https://c88e2772.typertiangolo.pages.dev

Copy link

github-actions bot commented Aug 9, 2024

@tiangolo
Copy link
Member

Awesome, thank you @svlandeg! Thanks for all the clear explanations, clean code, and help with everything. 🙇 🚀

This will be available in Typer 0.12.5 released in the next few hours. 🎉

@tiangolo tiangolo merged commit fc2c54f into fastapi:master Aug 24, 2024
20 checks passed
@svlandeg svlandeg deleted the fix/rich_none branch August 26, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants