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

RST cannot contain header only - cause silent crash #149

Closed
kamichal opened this issue Mar 2, 2019 · 3 comments · Fixed by #231
Closed

RST cannot contain header only - cause silent crash #149

kamichal opened this issue Mar 2, 2019 · 3 comments · Fixed by #231

Comments

@kamichal
Copy link

kamichal commented Mar 2, 2019

If the source README.rst contains header only, e.g.:

Header
======

The twine check command claims the markup is invalid for no reason:

twine check 'dist/*'
Checking distribution dist/foo-0.0.1.tar.gz: Failed
The project's long_description has invalid markup which will not be rendered on PyPI. The following syntax errors were detected:

Without listing any errors. Thats the end of stdout/stderr.

If I change the README.rst by adding just single letter at end, e.g.:

Header
======
a

or clear it contents everything is ok:

twine check 'dist/*'
Checking distribution dist/grot-0.0.1.tar.gz: Passed

I debugged it a bit. And got to the line 99 in site-packages/readme_renderer/rst.py:
rendered = parts.get("docinfo", "") + parts.get("fragment", "")
and the rendered string is empty. (both docinfo and fragment contain empty strings).
It corrupts the later if:

    if rendered:
        return clean(rendered)
    else:
        return None

...causing render function return None, which is interpreted by twine.check as error, but no warnings/errors are collected by used stream.

@miketheman
Copy link
Member

Looked at this a bit, and discovered that a header-only .rst file emits an H1 html tag, which is filtered out by design via setting (from 8 years ago):

# Set our initial header level
"initial_header_level": 2,

(overrides docutils default: https://docutils.sourceforge.io/docs/user/config.html#writer-specific-defaults-2 )

It's possible that is intentional - to prevent duplication of the project title inside the body, but it does highlight a difference between the rst and markdown render behaviors.
I found an example of this on my own library, when I switched from rst to md between versions https://pypi.org/project/pytest-socket/0.3.5/ vs https://pypi.org/project/pytest-socket/0.4.0/

But that's an issue for another time!


The bug here, as it appears to me, is that twine didn't emit a warning or error message to help the user understand what went wrong - as the reporter said - a "silent crash".
And twine can't know if there's a warning, if this library doesn't emit one.

miketheman added a commit to miketheman/readme_renderer that referenced this issue Mar 13, 2022
@di
Copy link
Member

di commented Mar 14, 2022

It's possible that is intentional - to prevent duplication of the project title inside the body, but it does highlight a difference between the rst and markdown render behaviors.

It's intentional! For exactly the reason you highlighted.

@kamichal
Copy link
Author

It's intentional! For exactly the reason you highlighted.

Then the error should be given and possibly explained when it crashes with such an "intention".

miketheman added a commit to miketheman/readme_renderer that referenced this issue Mar 20, 2022
miketheman added a commit to miketheman/readme_renderer that referenced this issue Mar 23, 2022
@di di closed this as completed in #231 Apr 18, 2022
di pushed a commit that referenced this issue Apr 18, 2022
* fix: emit a warning when no content is rendered

Fixes #149

Signed-off-by: Mike Fiedler <[email protected]>

* feat: emit empty warning only if no warnings exist

This is a bit more gymnastics than I think is warranted,
but wanted to play out the example.

Needing to inspect the stream proved more complex than
originally expected, as each version confroms to a slightly
different interface.

- Updated the outpout string to be clearer
- Added test for empty RST file

Signed-off-by: Mike Fiedler <[email protected]>

* refactor: simplify now that distutils is gone

Signed-off-by: Mike Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants