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

✅ Add more tests for the behaviour of rich_markup_mode #964

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

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
Comment on lines +54 to +58
pytest.param(
"rich",
["First line", "", "Line 1", "", "Line 2", "", "Line 3", ""],
marks=pytest.mark.xfail,
),
Copy link
Member Author

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

Comment on lines +172 to +174
pytest.param(
"rich", ["First line", "", "- 1 - 2 - 3", ""], marks=pytest.mark.xfail
),
Copy link
Member Author

@svlandeg svlandeg Dec 26, 2024

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.

Comment on lines +212 to +216
pytest.param(
"rich",
["First line", "", "- 1", "", "- 2", "", "- 3", ""],
marks=pytest.mark.xfail,
),
Copy link
Member Author

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

Comment on lines +257 to +261
pytest.param(
"rich",
["First line", "", "- 1 - 2 - a - b - 3", ""],
marks=pytest.mark.xfail,
),
Copy link
Member Author

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.

Comment on lines +302 to +306
pytest.param(
"rich",
["First line", "", "- 1", "", "- 2", "", "- a", "", "- b", "", "- 3", ""],
marks=pytest.mark.xfail,
),
Copy link
Member Author

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal repo / tests Involving the CI / test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants