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

🐛 Fix shell completions for the fish shell #1069

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

goraje
Copy link

@goraje goraje commented Nov 29, 2024

As described in this discussion #1068, recent changes aimed at sanitizing the help text from rich tags during autocompletion generation (#877) have caused problems with proper generation of autocompletions for fish shell, when rich is installed in the environment. The main culprit most likely being the rich.Console.print method that converts the tabs into spaces which results in malformed autocompletes. I propose a different approach to sanitizing the help text, one that allows to perform the removal of tags before the output of format_completion gets returned, thus allowing to generate a rich tag-free help text, while still being able to pass it to a formatting-preserving output via click.echo in typer.completion (basically reverting this part of the source to the way it was before the #877 changes).

An example of the current behaviour while using rich.Console.print invoked in typer.rich_utils.print_with_rich:

testapp example1\ \ \ Just\ some\ help\ for\ example1

with an example app of the following structure:

from typing import Annotated

import typer
from rich.markup import escape

app = typer.Typer(no_args_is_help=True, rich_markup_mode="rich")

@app.command(name="example1", help="Just [bold]some[/] help for example1")
def example1(name: Annotated[str, typer.Option(help="name")]) -> None:
    pass

@app.command(name="example2", help=f"Just some help for example2 {escape('[I'm just text in brackets]')}")
def example2() -> None:
    pass

def main() -> None:
    app()

@goraje goraje force-pushed the fix-fish-shell-completions branch 2 times, most recently from 5e44213 to eda9ccb Compare November 29, 2024 23:22
@goraje goraje force-pushed the fix-fish-shell-completions branch from eda9ccb to b22f193 Compare November 29, 2024 23:32
@svlandeg svlandeg self-assigned this Dec 2, 2024
@svlandeg svlandeg added the bug Something isn't working label Dec 2, 2024
@svlandeg
Copy link
Member

svlandeg commented Dec 4, 2024

Thanks for the detailed report and PR! I'll look into this 🙏

@svlandeg svlandeg changed the title Fix fish shell completions 🐛 Fix shell completions for the fish shell Dec 4, 2024
Copy link
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.

Hi @goraje,

Thanks so much for this PR and the detailed report! The issue is clear and it does look like a regression for the fish shell.

In the PR #877, I should have added some more tests to check this behaviour for different shells. I've now gone in and added these tests to your PR: I've created a (minimal) version of docs_src.commands.help.tutorial001 that has Rich tags in its subcommand descriptions, and ran completion tests in the new file test_completion_complete_rich.py to double check that the Rich formatting isn't there. These tests seem to be succeeding just fine on this PR. I double checked on master, and there the test_completion_complete_subcommand_fish fails because of the tab substitution you mentioned.

I saw that you also added tests to double check the functionality of the _sanitize_help_text function. I think that's great - I've just gone ahead and pulled that into its own separate test file for clarity.

So far the behaviour and code look fine to me, but I'll yet perform a final detailed review of all the code changes and will comment here again when I do so.

Copy link
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.

This looks good to me, leaving this for a final review by Tiangolo. Thanks again, @goraje!

@svlandeg svlandeg removed their assignment Dec 19, 2024
@goraje
Copy link
Author

goraje commented Dec 19, 2024

Thank you for looking into that :) I completely agree that the tests didn’t quite belong but I didn’t feel confident enough to modify the project’s structure by adding a test module on my own so thank you for fixing that for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working shell / fish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants