-
-
Notifications
You must be signed in to change notification settings - Fork 726
🐛 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
Conversation
There was a problem hiding this 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.
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:
|
…docstring_tests # Conflicts: # tests/test_rich_markup_mode.py
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 With respect to the (new) test with docstring
the lines of the body will be merged together for |
rich_markup_mode
rich_markup_mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @svlandeg ! 🚀 🍰
Also, this PR now already includes the quick bug fix that ensures there's a newline between the header and the body. |
This is available now in Typer 0.15.3, just released 🚀 |
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 onmaster
. Any potential fixes will then be contributed in subsequent PRs.