-
-
Notifications
You must be signed in to change notification settings - Fork 684
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
✅ Add more tests for the behaviour of rich_markup_mode
#964
base: master
Are you sure you want to change the base?
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.
pytest.param( | ||
"rich", | ||
["First line", "", "Line 1", "", "Line 2", "", "Line 3", ""], | ||
marks=pytest.mark.xfail, | ||
), |
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.
This one fails on master
only because of a missing ""
right after the header
pytest.param( | ||
"rich", ["First line", "", "- 1 - 2 - 3", ""], marks=pytest.mark.xfail | ||
), |
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.
Note that currently on master
, the output for this one is
['First line', '- 1', '- 2', '- 3', '']
This may mean that some users could see it as a "breaking" change once we change the code to succeed this test.
pytest.param( | ||
"rich", | ||
["First line", "", "- 1", "", "- 2", "", "- 3", ""], | ||
marks=pytest.mark.xfail, | ||
), |
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.
This one fails on master
only because of a missing ""
right after the header
pytest.param( | ||
"rich", | ||
["First line", "", "- 1 - 2 - a - b - 3", ""], | ||
marks=pytest.mark.xfail, | ||
), |
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.
Note that currently on master
, the output for this one is
['First line', '- 1', '- 2', '- a', '- b', '- 3', '']
This may mean that some users could see it as a "breaking" change once we change the code to succeed this test.
pytest.param( | ||
"rich", | ||
["First line", "", "- 1", "", "- 2", "", "- a", "", "- b", "", "- 3", ""], | ||
marks=pytest.mark.xfail, | ||
), |
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.
This one fails on master
only because of a missing ""
right after the header
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:
|
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.