🐛 Fix escaping in help text when rich is installed but not used

[svlandeg]

Background

With PR #847, we implemented functionality to ensure that help descriptions with Rich tags (e.g. [bold]) would get rendered properly in Markdown (instead of just showing the "raw" brackets). This uses Rich's functionality from console.export_html.

Unfortunately, a problem occurred at the time because meta information like [required] looks like Rich tags, but with no known formatting they would just get stripped off by console.export_html(). That then meant we had to escape those tags at the point in the code where they're being added to the rest of the help string - this happens in get_help_record for both TyperOption and TyperArgument :

if rich is not None:
    # This is needed for when we want to export to HTML
    extra_str = rich.markup.escape(extra_str).strip()

help = f"{help}  {extra_str}" if help else f"{extra_str}"

Current issue

Now, as pointed out in #1058, there's now an issue when the escaping happens if rich is installed (triggering the if clause in the code above) BUT the Typer app actually has rich_markup_mode=None. The help text will in this case show the backslash from the escaping, which is clearly unwanted behaviour.

Fix (proposal)

To remedy, we need to know at the time of escaping what the value is of rich_markup_mode. This is not trivial, as TyperOption and TyperArgument are unaware of this information. And we can't just pass it through from the Typer app either, because get_click_param() creates empty/default OptionInfo/ArgumentInfo objects which will lead to default values downstream. I don't see a way to make things work via this route.

Instead, this PR proposes to store a custom value in ctx.obj. This object is being passed around anyway, and as far as I understood, ctx.obj may be used for these kind of application-specific settings. The code I've written tries to be super careful not to mess up any current usages of this object:

    if hasattr(typer_obj, "rich_markup_mode"):
        if not hasattr(ctx, "obj") or ctx.obj is None:
            ctx.ensure_object(dict)
        if isinstance(ctx.obj, dict):
            ctx.obj['TYPER_RICH_MARKUP_MODE'] = typer_obj.rich_markup_mode

Then, right before the escaping we can check this value and refrain from escaping if Rich is not enabled.

Additional complication

When utils docs is called to create a markdown version of a file, we need to make sure that we only call rich_to_html on text that has been properly escaped, i.e. when rich is installed AND rich_markup_mode == "rich". This requires us to inspect the value of ctx.obj also in the function _parse_html. I've added another unit test to double check this behaviour when rich_markup_mode=None - cf test_doc_no_typer()

[svlandeg]

This fails with master, as pointed out in #1058 (it'll have a slash)

[svlandeg]

The point of this new test is this: checking that rich_markup_mode=None, with rich installed, still produces correct results (showing [required], default values, etc and not accidentally 'removing' them because of incorrect escaping behaviour)

[patrick91]

🤔 I think we should move the computation of rich_markup_mode in get_docs_for_click so we don't do it more than necessary (and we keep this function simple)

[svlandeg]

Yea fair enough, I had it there first 😁 Somehow that felt like cluttering the get_docs_for_click function, and conceptually it's nicer to pass around ctx everywhere. But I agree that it means repeating the same computation (though they should be simple, fast checks/retrievals)

[svlandeg]

We should also consider pulling this "TYPER_RICH_MARKUP_MODE" value out as a variable and putting it somewhere central.