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

Use Markdown and text renderers in check #876

Closed
bhrutledge opened this issue Feb 20, 2022 · 6 comments
Closed

Use Markdown and text renderers in check #876

bhrutledge opened this issue Feb 20, 2022 · 6 comments

Comments

@bhrutledge
Copy link
Contributor

bhrutledge commented Feb 20, 2022

Related to #875, I was reminded that Twine doesn't actually "check" these file types:

_RENDERERS = {
None: readme_renderer.rst, # Default if description_content_type is None
"text/plain": None, # Rendering cannot fail
"text/x-rst": readme_renderer.rst,
"text/markdown": None, # Rendering cannot fail
}

However, Readme Renderer has renderers for these file types. On the surface, it looks like they're unlikely to issue warnings, but it seems like using them would be harmless and potentially beneficial in the future.

@di as an author of check and maintainer of Readme Renderer, what do you think?

@di
Copy link
Sponsor Member

di commented Feb 21, 2022

For text/plain and text/markdown, rendering cannot fail (as the comments note) so there's nothing that will prevent a description with these content types from being uploaded, so there's nothing for twine check to check.

@sigmavirus24
Copy link
Member

Maybe we need better comments explaining why rendering cannot fail so that this is more easily self-discoverable in the future?

@bhrutledge
Copy link
Contributor Author

Is there any harm in running them anyway? Maybe those renderers could fail in the future?

@di
Copy link
Sponsor Member

di commented Feb 21, 2022

Is there any harm in running them anyway? Maybe those renderers could fail in the future?

I don't think there's any harm, per se, but it's just strictly not necessary and I can't imagine it ever being necessary.

One of the advantages to these formats, specifically Markdown, is that they can still render all the valid syntax in a document even if there is a syntax error or some content is malformed (unlike reStructuredText, where rendering is a pass/fail). So, any change that would alter this sort of fundamental property of Markdown.

If such a change ever did happen, I'd definitely be opposed to including it at any point in our Markdown rendering pipeline. I think it's important for PyPI to continue accepting any Markdown, no matter what it produces.

Similarly plain text is just plain text -- the actual 'rendering' is a no-op here.

The other things we do to these formats (html escaping, cleaning, etc) will also never cause the render to fail, so running them is unnecessary as well.

@sigmavirus24
Copy link
Member

What's your goal here @bhrutledge ? What problem are you trying to fix?

@bhrutledge
Copy link
Contributor Author

What problem are you trying to fix?

A combination of not understanding a perceived discrepancy between Twine and Warehouse, and possibly future-proofing Twine. I understand now, and it turns out that using the Markdown renderer requires installing readme_renderer[md] (ie. cmarkgfm) which seems excessive.

Thanks @di for the explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants