Skip to content

🐛 Make sure rich_markup_mode=None disables Rich formatting#726

Closed
liambeguin wants to merge 11 commits intofastapi:masterfrom
liambeguin:rich_markup_mode
Closed

🐛 Make sure rich_markup_mode=None disables Rich formatting#726
liambeguin wants to merge 11 commits intofastapi:masterfrom
liambeguin:rich_markup_mode

Conversation

@liambeguin
Copy link
Copy Markdown

@liambeguin liambeguin commented Dec 22, 2023

As stated in the documentation, rich_markup_mode=None is the default mode.
Make sure that this is true even when rich is installed, fix current tests assuming it's on by default, and add new test cases to validate rich_markup_mode options.

@liambeguin
make sure rich_markup_mode=None disables rich
1579346 
As stated in the documentation, rich_markup_mode=None is the default
mode. Make sure that this is true even when rich is installed, fix
current tests assuming it's on by default, and add new test cases to
validate rich_markup_mode options.

Signed-off-by: Liam Beguin <[email protected]>

🎨 [pre-commit.ci] Auto format from pre-commit.com hooks
@JacobKochems JacobKochems mentioned this pull request Dec 23, 2023
Closed
@svlandeg svlandeg added feature New feature, enhancement or request p3 labels Feb 28, 2024
@svlandeg svlandeg added bug Something isn't working p2 and removed feature New feature, enhancement or request p3 labels Jun 10, 2024
@svlandeg svlandeg changed the title make sure rich_markup_mode=None disables rich 🐛 Make sure rich_markup_mode=None disables Rich formatting Jun 10, 2024
svlandeg
svlandeg reviewed Jun 10, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Using the example code snippet from #853, I can confirm that on master and in an environment with Rich installed, rich_markup_mode=None does not seem to have the desired effect as described in the documentation:

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

This PR fixes that, and effectively disables Rich for the given Typer app.

I'm putting this PR in draft until the issues with coverage are resolved - as some of the tests have been changed, there are code paths no longer covered by the test suite, and we need to fix that before we can merge this.

@svlandeg svlandeg marked this pull request as draft June 10, 2024 15:45
svlandeg
svlandeg reviewed Jun 11, 2024
View reviewed changes
Comment thread typer/core.py Outdated
@svlandeg svlandeg self-assigned this Jun 11, 2024
svlandeg
svlandeg reviewed Jun 11, 2024
View reviewed changes
Comment thread tests/test_tutorial/test_arguments/test_help/test_tutorial006.py
@svlandeg svlandeg mentioned this pull request Jun 12, 2024
Merged
@svlandeg
Copy link
Copy Markdown
Member

svlandeg commented Jun 12, 2024

Hi @liambeguin or anyone else - if you have time, I'd appreciate a review of #859, which I've opened as an alternative to this PR (building on the same idea but without changing the defacto default).

@liambeguin
Copy link
Copy Markdown
Author

liambeguin commented Jun 12, 2024

Hi @liambeguin or anyone else - if you have time, I'd appreciate a review of #859, which I've opened as an alternative to this PR (building on the same idea but without changing the defacto default).

Hi @svlandeg, thanks for taking the time to look at this. I was a bit confused by the need for a second PR after all the traffic on this one, but if I understand correctly #859 does the same thing but keeps the old behaviour as default and fixes the documentation? I don't mind either way, as long as I can disable it.

Also, I think it would be best to not duplicate PR like this and edit the first one once a consensus is reached.

@svlandeg
Copy link
Copy Markdown
Member

svlandeg commented Jun 12, 2024 •
edited
Loading

I was a bit confused by the need for a second PR after all the traffic on this one, but if I understand correctly #859 does the same thing but keeps the old behaviour as default and fixes the documentation?

That's right. It resolves the same original bug, but does not change the current default behaviour. Instead it updates the documentation to reflect it correctly.

Also, I think it would be best to not duplicate PR like this and edit the first one once a consensus is reached.

I get that. I did spent a great deal of time looking at your PR to update it with the latest master, fix some issues with it, and try to resolve the remaining coverage problems. Ultimately this PR would require many more fixes to the test suite to get it in shape, as most of the tests assume the default behaviour has rich enabled - you can find all of these files by looking for functions with "no_rich" in their name. So as I was diving deeper and deeper into this, and realised how many more changes this would need, I realised that this change would also be quite breaking for other users. And then it dawned on me that another alternative is keeping the default as-is and create a fix with minimal required changes - i.e. PR #859.

I am leaving these two PRs open side by side so that Tiangolo can clearly see both opions and can pick either of the two.

@liambeguin
Copy link
Copy Markdown
Author

liambeguin commented Jun 12, 2024

Oh I see! well I appreciate you taking the time to fix the coverage tests :)

It might make more sense to do it the way you did then.

@svlandeg svlandeg removed their assignment Jun 12, 2024
@tiangolo
Copy link
Copy Markdown
Member

tiangolo commented Aug 24, 2024

Thanks @liambeguin! ☕

This was handled in #859

The fix will be available in Typer 0.12.5, released in the next few hours. 🎉

Thanks a lot @svlandeg for all the help as always! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@svlandeg svlandeg svlandeg left review comments

Assignees

No one assigned

Labels

bug Something isn't working p2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@liambeguin @svlandeg @tiangolo