🐛 Make sure rich_markup_mode=None disables Rich formatting

[liambeguin]

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.

[svlandeg]

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]

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]

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]

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]

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.

[tiangolo]

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! 🙇