search

pypa / twine

Utilities for interacting with PyPI
https://twine.readthedocs.io/
Apache License 2.0
1.61k stars 308 forks source link

Use Markdown and text renderers in `check` #876

Closed bhrutledge closed 2 years ago

bhrutledge commented 2 years ago

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

https://github.com/pypa/twine/blob/0f7a68bbf556478b8b5360aa02b1291775d92182/twine/commands/check.py#L28-L33

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 commented 2 years ago

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 commented 2 years ago

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

bhrutledge commented 2 years ago

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

di commented 2 years ago

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 commented 2 years ago

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

bhrutledge commented 2 years ago

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.