8000 🐛 Fix newline after header in help text, and add more tests for the behaviour of `rich_markup_mode` by svlandeg · Pull Request #964 · fastapi/typer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

🐛 Fix newline after header in help text, and add more tests for the behaviour of rich_markup_mode #964

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

Merged
merged 15 commits into from
Mar 20, 2025

Conversation

svlandeg
Copy link
Member
@svlandeg svlandeg commented Sep 2, 2024

There's a few open PRs around markdown formatting and parsing that I want to review, but first I would like to establish what exactly is the desired outcome for different docstrings and different settings of rich_markup_mode.

This PR is supposed to capture the ideal output for each test, using a pytest.mark.xfail marker for those cases that currently fail on master. Any potential fixes will then be contributed in subsequent PRs.

Copy link
Member
@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Great idea to have more tests and a more established and expected behavior!

I added some comments in your comments, thinking about the best way to proceed with unwrapping lines or not.

Now I was just reading the docstrings PEP: https://peps.python.org/pep-0257/, it mentions that the first line should be a "summary", followed by a blank line, and that it should fit on a single line.

So I guess that for the first line it's up to us.

The docstrings PEP doesn't say anything about wrapping long lines or not, or how to consider it.

But PEP 8 (https://peps.python.org/pep-0008/#maximum-line-length) does, and explicitly mentions docstrings:

[...] it is okay to increase the line length limit up to 99 characters, provided that comments and docstrings are still wrapped at 72 characters.

So I guess it's kind of expected that people would wrap docstring content with internal new lines without signifying that it means new paragraphs.

So, although I don't like it too much, I'm inclined to not support lists separated by a single line and require them to have blank lines in between.

@svlandeg svlandeg assigned svlandeg and unassigned tiangolo Nov 28, 2024
@svlandeg svlandeg marked this pull request as draft December 26, 2024 10:57
@svlandeg
Copy link
Member Author

Hi @tiangolo,

Thanks for the detailed feedback!

I agree that the trade-off is difficult here, because some users may "feel like" single lines should be unwrapped, while others may feel like they should remain separated, e.g. when using lists. I don't think we'll be able to satisfy all use-cases, but at least aiming for consistency will be good.

I've edited all tests to try and reflect your feedback - could you please go through them one final time to double check this is how you intend it?

Two important points that I've also highlighted in individual comments:

  • For the tests with lists & double lines, rich formatting is mostly OK already on master, except that there's currently a missing newline right after the header. From your very first comment on the first test test_markup_mode_newline_pr815 I gather that you always want this newline to be present in the output after the header, correct?
  • For the tests with lists & single lines, note that the rich formatting option is currently parsing the lists and not unwrapping the single newline. If I understood you correctly, we want to reverse this behaviour, glue the lines together, and only support lists when there's double lines. This is what the tests currently reflect. However, I could imagine some users taking issue with this decision, because it will mean that their specific use-case (lists with single line) may break in a future version of Typer. If this is what we decide, we will need to communicate about this clearly.

@svlandeg svlandeg marked this pull request as ready for review December 26, 2024 11:38
@svlandeg svlandeg removed their assignment Dec 26, 2024
@svlandeg
Copy link
Member Author 8000

Ok, after internal discussion with @tiangolo, we've decided on the state of things as they're currently represented by the tests. In contrast to the earlier discussion, we've decided to support lists with single line breaks anyway, as rich is already capable of doing this and we wouldn't need complicated logic on our end.

With respect to the (new) test with docstring

        """
        First line

        Line 1
        Line 2
        Line 3
        """

the lines of the body will be merged together for markdown and None mode, but not for Rich mode, mimicking more closely what native Rich does.

@svlandeg svlandeg changed the title ✅ Add more tests for the behaviour of rich_markup_mode 🐛 Fix newline after header in help text, and add more tests for the behaviour of rich_markup_mode Mar 20, 2025
@svlandeg svlandeg added bug Something isn't working and removed internal labels Mar 20, 2025
Copy link
Member
@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Thank you @svlandeg ! 🚀 🍰

@svlandeg
Copy link
Member Author

Also, this PR now already includes the quick bug fix that ensures there's a newline between the header and the body.

@tiangolo tiangolo merged commit bd062bc into fastapi:master Mar 20, 2025
29 checks passed
@svlandeg svlandeg deleted the feat/docstring_tests branch March 20, 2025 12:24
@tiangolo
Copy link
Member

This is available now in Typer 0.15.3, just released 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working repo / tests Involving the CI / test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0