Skip to content

🐛 Fix shell completions for the fish shell#1069

Merged
tiangolo merged 5 commits intofastapi:masterfrom
goraje:fix-fish-shell-completions
Feb 27, 2025
Merged

🐛 Fix shell completions for the fish shell#1069
tiangolo merged 5 commits intofastapi:masterfrom
goraje:fix-fish-shell-completions

Conversation

@goraje
Copy link
Copy Markdown
Contributor

@goraje goraje commented Nov 29, 2024 •
edited
Loading

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
Copy Markdown
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
svlandeg
svlandeg reviewed Dec 19, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@svlandeg svlandeg left a comment •
edited
Loading

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.

svlandeg
svlandeg approved these changes Dec 19, 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.

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
Copy Markdown
Contributor 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 :)

@ndrew222
Copy link
Copy Markdown

ndrew222 commented Jan 12, 2025

It would be great if this can be merged soon, it's been undermining my use of the nala program.
Thanks so much for your good work

@pilucca
Copy link
Copy Markdown

pilucca commented Jan 14, 2025

Hi, is it possible to have any update about this issue? Thanks

tiangolo
tiangolo approved these changes Feb 27, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tiangolo tiangolo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome, thank you! 🚀 🎉

I was able to confirm the bug with the included Docker Compose and then confirm the fix the same way, using the included test file.

Thanks @goraje for the detailed report. 🍰

Thanks @svlandeg for the thorough review. 🙌

This will be available in the next version, 0.15.2 released in the next hours. 🚀

@tiangolo tiangolo merged commit 4f04666 into fastapi:master Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiangolo tiangolo tiangolo approved these changes

@svlandeg svlandeg svlandeg approved these changes

Assignees

No one assigned

Labels

bug Something isn't working shell / fish

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@goraje @svlandeg @ndrew222 @pilucca @tiangolo