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

🐛 Fix markdown formatting #815

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

gar1t
Copy link

@gar1t gar1t commented Apr 30, 2024

Includes fix to typo in function name.

Ref: #447

Includes fix to typo in function name.

Ref: fastapi#447
@svlandeg svlandeg added bug Something isn't working p2 labels May 2, 2024
@svlandeg svlandeg linked an issue May 21, 2024 that may be closed by this pull request
7 tasks
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.

Thanks for the PR!

Before I review the actual code edits, I want to take a step back and better understand what the desired output would be. In the linked issue #447, the author uses this example:

@app.command(name="tester-cmd", help="""
    Header

    Line 1
    Line 2
    Line 3
""")

which they want to get parsed (expected output) as

Header                                                                                                                                                                     
Line 1 
Line 2
Line 3   

However, with this PR, the output would actually be

 Header

 Line 1 Line 2 Line 3

(at least on my Windows system)

While currently on master it's

 Header
 Line 1 Line 2 Line 3

With mode rich, the output on master is

 Header
 Line 1
 Line 2
 Line 3

and this PR changes that to

 Header

 Line 1
 Line 2
 Line 3

Is that what we want?

Basically, I suggest that we first extend the test suite to cover various versions of the test (different number of newlines and different modes) and record which is the expected output for each. Then, we can look into how to get that fixed for all cases.

tests/test_rich_utils.py Outdated Show resolved Hide resolved
@gar1t
Copy link
Author

gar1t commented May 21, 2024

The source:

Header

Line 1
Line 2
Line 3

I think ought to have a space after Header (as per the PR). That's my thought based on how Markdown is typically formatted in HTML.

E.g. while this is just one HTML formatter, it renders space between Header and the subsequent text block.

https://markdowntohtml.com/

I suspect it will be very hard to find any Markdown-to-HTML formatter that doesn't do this. I'd be curious to see it!

On this basis, I don't think the PR is controversial.

I would re-engage the OP for #447 and get his or her thoughts.

@svlandeg
Copy link
Member

The source:

Header

Line 1
Line 2
Line 3

I think ought to have a space after Header (as per the PR).

Can you elaborate? With this PR, that input gives me

 Header

 Line 1 Line 2 Line 3

There's a space after Header, but the lines still aren't separated. Is that what you'd expect? I'm asking because the current unit test doesn't cover this case.

@gar1t
Copy link
Author

gar1t commented May 21, 2024

I agree, whatever the agreed upon formatting behavior, the test case should include that source to put the matter to rest per the OP issue.

I should correct my comment above: not all markdown-to-html formatters treat text blocks the same. There are two ways to treat text block that I'm seeing:

  1. A single line-ending to terminate the block
  2. Two line-endings terminate the block

The GitHub formatter, e.g. uses method 1. You can try this by pasting the source and previewing it. The lines appear on separate lines.

This formatter uses method 2. If you paste the source in you'll see the text block is treated as one block. The lines don't appear separately.

The same is true for Python's markdown module:

> echo "Header 1
::: 
::: Line 1
::: Line 2
::: Line 2" | python -m markdown
<p>Header 1</p>
<p>Line 1
Line 2
Line 2</p>

I would still argue that typer should implement method 2. I think this is typical for tools and source code (where editors help enforce max line lengths with formatting), whereas the GitHub formatter is suited for human editing in simple text areas (like this one!)

I'm happy to add that test case.

@svlandeg
Copy link
Member

svlandeg commented May 28, 2024

I'm happy to add that test case.

That would be great, yes! Looking at some detailed unit tests will help us decide what the final behaviour is we want/need.

Update: we could potentially also included the cases reported in #449.

@svlandeg svlandeg marked this pull request as draft June 3, 2024 08:35
@svlandeg svlandeg self-assigned this Aug 5, 2024
@svlandeg
Copy link
Member

svlandeg commented Sep 2, 2024

Update: I've solicited feedback from Tiangolo on PR #964 to first establish what the ideal output is for a variety of use-cases. This will help us better understand the changes proposed in this PR as well.

I know that some of these edits may seem straightforward, but as I was writing the additional unit tests it became clear to me that there are a lot of different edge cases and interdependencies of the code paths, so I think it's important to get this all straight first.

I will get back to this PR once we've decided on the other one. Thanks for your patience! 🙏

@svlandeg svlandeg changed the title Fix markdown formatting 🐛 Fix markdown formatting Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential bug with new lines in the help output when markdown mode is used
2 participants