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

💄 Unify the width of the Rich console for help and errors #788

Merged
merged 3 commits into from
Aug 24, 2024

Conversation

racinmat
Copy link
Contributor

@racinmat racinmat commented Apr 2, 2024

Fixes #787 by unifying the behavior of consoles and passing the width parameter consistently.
As this will change the default behavior, should I make some feature flag so the new behavior is opt-in, or is it ok like this?

@svlandeg svlandeg added feature New feature, enhancement or request p3 labels Apr 2, 2024
@racinmat
Copy link
Contributor Author

@svlandeg hi, do you have some ETA to get this merged? Or is there something else I should/could do to get this merged?

@jsilva
Copy link

jsilva commented Jul 1, 2024

@svlandeg, what is blocking this PR ?

We would like to finish the circle on this fix so our users have a better experience.

Thank you

@racinmat
Copy link
Contributor Author

racinmat commented Jul 4, 2024

ping @tiangolo

@svlandeg svlandeg changed the title fix: unifies the rich console between traceback and CLI 💄 Unify the width of the Rich console Aug 5, 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 @racinmat, thanks for this PR!

It makes a lot of sense to unify the Rich console appearence, and call the rich_utils._get_rich_console() function to define the console_stderr. This seems to be the original intention as well, as this function takes an explicit stderr boolean argument.

Setting the width to MAX_WIDTH instead of taking the default of 100 also makes more sense to me, and it indeed looks nicer on the console.

I can confirm on my system that the width of the help info box and the width of the error box differ on master but are the same with this PR. It definitely comes across as more unified and clean this way.

Finally - on the point of whether or not this is breaking change and would require a feature switch: while the default behaviour does change, it's not really a breaking API change IMO, so I would probably merge this as-is. I'll leave the final decision on this with Tiangolo though.

@tiangolo tiangolo changed the title 💄 Unify the width of the Rich console 💄 Unify the width of the Rich console for help and errors Aug 24, 2024
@tiangolo
Copy link
Member

Great! Makes sense, thank you! @racinmat! 🍰

And thanks @svlandeg as always. 🙇

@tiangolo tiangolo merged commit 493dd8e into fastapi:master Aug 24, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature, enhancement or request p3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants