fix: emit a warning when no content is rendered

[miketheman]

Fixes #149

Signed-off-by: Mike Fiedler miketheman@gmail.com

[miketheman]

cc @bhrutledge - the bug surfaced via a twine check command - see original issue.
This library hadn't previously emitted any specific warnings, rather passing through any received from docutils, so this is the first that I can tell where we're emitting our own to the stream.

[bhrutledge]

@miketheman Thanks for sharing, and fixing! I'd like to take a look at how this displays in Twine, and see if there could be a more helpful message. I should be able to do that later today.

[bhrutledge]

I think this could use some more work. Given a README.rst like:

Header
======

The latest (unreleased) version of Twine shows:

First, it'd be nice if the no output rendered message were more helpful, e.g. with a suggested resolution.

Furthermore, this new output also shows up for RST syntax errors. With a spurious header line:

Header
======

======

The output is:

Compared to readme_renderer==34.0:

---

Also, I wonder if the scenario described in #149 (automatically removed header) could/should be treated the same way as an empty README.rst, i.e., passing with an empty description:

[miketheman]

@bhrutledge Thanks for testing this branch - these examples are very helpful!

Considering that the condition expressed in the added test case is intentional behavior, would it make sense to not apply this change, and rather change how twine check behaves, rather than this library?
Maybe around https://github.com/pypa/twine/blob/a0ba32dcd0ea5af5de7b88d6645b7743bf003760/twine/commands/check.py#L102-L103 - check if there are warnings on the stream as well? This would then preserve the desired behavior of readme_renderer to emit the warnings from the docutils dependency it uses.

Then the second example with the spurious header line would still work as it's supposed to.

[bhrutledge]

Twine could check the warning stream, but if I follow this code, so could readme_renderer, and add a helpful warning if there's no content. I think I have a preference for the latter, to make this transparent to Twine, and useful in other projects that use this package. What do you think?

[miketheman]

Fair enough - I figured if the original report was expressing dissatisfaction with their twine check results, maybe twine would answer their need. 😉

I'll give it a shot - checking the stream for any warnings from docutils first, and if there are, leave them be, but if there are none, then add our warning.

Do you have any suggestions for a better warning message?

[bhrutledge]

How about something like "No content rendered from RST source.\n"? That's still a little vague/awkward, but at this point in the code we don't where the actual raw content is coming from. In Twine, it's not much better; it's just reading the description from the package metadata.

Considering that the condition expressed in the added test case is intentional behavior

I dug into this, and discovered some interesting behavior. Given two packages, one with an empty README.rst, and one with only a header:

% tail dist/example-rst-empty-0.0.5/PKG-INFO 
Description-Content-Type: text/x-rst
License-File: LICENSE

UNKNOWN

% tail dist/example-rst-header-0.0.5/PKG-INFO
Description-Content-Type: text/x-rst
License-File: LICENSE

Header
======

Twine will pass empty, but fail header:

% twine check dist/*.gz
Checking dist/example-rst-empty-0.0.5.tar.gz: PASSED
Checking dist/example-rst-header-0.0.5.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                
         No content rendered from RST source.

However, via some print debugging, it seems readme_renderer treats both as an error:

% python3 -m readme_renderer dist/example-rst-empty-0.0.5/README.rst; echo Exit: $? 
No content rendered from RST source.
raw = ''
Exit: 1

% python3 -m readme_renderer dist/example-rst-header-0.0.5/README.rst; echo Exit: $?
No content rendered from RST source.
raw = 'Header\n======\n'
Exit: 1

Looking at the Twine source, I think this actually exposes a bug. It seems that there’s a check for an UNKNOWN description, but with 3 trailing newlines, while the metadata only has 2. That results in a call to render(), and a “valid” result:

% twine check dist/example-rst-empty-0.0.5.tar.gz           
Checking dist/example-rst-empty-0.0.5.tar.gz: 
description = 'UNKNOWN\n\n', rendering_result = '<p>UNKNOWN</p>\n', is_ok = True
PASSED

I don't know when this behavior changed in Twine, but I think it should be fixed. For now, with a quick hack, here’s the result:

% twine check dist/*.gz     
Checking dist/example-rst-empty-0.0.5.tar.gz: 
description = 'UNKNOWN\n\n', rendering_result = None, is_ok = True
PASSED with warnings
WARNING  `long_description` missing.                                                                                                      
Checking dist/example-rst-header-0.0.5.tar.gz: 
description = 'Header\n======\n\n\n', rendering_result = None, is_ok = False
FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                
         No content rendered from RST source.                                                                                             
         raw = 'Header\n======\n\n\n'

That looks right to me: empty generates a warning, but header is an error because the long_description won't be rendered as-is.

Before I went down this rabbit hole, I was wondering if readme_renderer should treat an empty RST file differently from one with only a header. However, from Twine's perspective, I don't think it matters. I do still think that it would be nice for readme_renderer to add a warning.

[miketheman]

@bhrutledge Thanks for digging in - super helpful!
I've pushed another commit (after rebase of the original one on the current main) - which implements the clearer message you suggested, and added some smarter detection of whether or not to emit a warning.

Glad that we exposed an interesting bug on twine as well - but considering that with this change readme_renderer will emit a warning for what it considers "empty" (after removing the first Header), does the UNKNOWN check still make sense over there? 🤔

[bhrutledge]

with this change readme_renderer will emit a warning for what it considers "empty" (after removing the first Header), does the UNKNOWN check still make sense over there? 🤔

I want to look into this a little more, but I think so, because otherwise readme_renderer will see UNKNOWN\n\n, and return <p>UNKNOWN</p\n. What's not clear to me at the moment is how that's rendered on PyPI, and the whole point of twine check is to avoid surprises in PyPI rendering.

[miketheman]

@bhrutledge I think this is now ready for re-review and release, considering that twine 4.0.0 is out with the changes to the stream.

[bhrutledge]

@miketheman Thanks for the nudge. It's been on my list, blocked by "real life". 😉 Hopefully this weekend.

[bhrutledge]

I think this is good! Here's the result from an empty README and a README with only a header:

% twine check dist/*.gz 
Checking dist/example-rst-empty-0.0.5.tar.gz: PASSED with warnings
WARNING  `long_description` missing.                                                                                                                                                             
Checking dist/example-rst-header-0.0.5.tar.gz: FAILED
ERROR    `long_description` has syntax errors in markup and would not be rendered on PyPI.                                                                                                       
         No content rendered from RST source.  

[bhrutledge]

Clever!

[miketheman]

Thanks! @di all yours now.