Markdown #51
Markdown #51
Conversation
Fixes: GH-1
Allows GitHub-style backtick syntax for code -- e.g.:
```python
import os
def add(a, b):
return a + b
```
Allows markup like:
Text with double__underscore__words.
And it will render the double underscores within the words instead of
taking them to mean to format as strong.
Solves PEP 8 line too long errors
Fixes the following test failure (e.g.: https://travis-ci.org/pypa/readme_renderer/jobs/148390785): ``` if "<" in expected: > assert out == expected E assert '<p>Here is s...nkey</a></p>\n' == '<p>Here is so...nkey</a></p>\n' E Skipping 734 identical leading characters in diff, use -v to show E - n class="s1">'Ruff!'</span><span class="p">)</span> E ? - E + n class="s">'Ruff!'</span><span class="p">)</span> E E - <span class="n">dog</span> <span class="o">=</span> <span class="n">Dog</span><span class="p">(</span><span class="s1">'Fido'</span><span class="p">)</span> E ? - E + <span class="n">dog</span> <span class="o">=</span> <span class="n">Dog</span><span class="p">(</span><span class="s">'Fido'</span><span class="p">)</span> E </pre> E Detailed information truncated (6 more lines), use "-vv" to show tests/test_rst.py:31: AssertionError ```
I guess the output of [Markdown](https://pypi.python.org/pypi/Markdown) changed a bit?
Also, the only case bool(markdown.markdown(?)) can give False looks like when given an empty string, itself returning and empty string. However, on very unparsable case, markdown.markdown() can raise a ValueError, let's catch it so we can fallback later on txt rendering.
This allow to replace occurrences of:
description_html = readme_renderer.rst.render(release["description"])
if description_html is None:
description_html = readme_renderer.txt.render(release["description"])
release["description_html"] = description_html
to simply:
release["description_html"] = readme_renderer.any.render(release["description"])
And gaining rendring or Markdown.
| @@ -0,0 +1,8 @@ | |||
| Here is some Python: | |||
|
|
|||
| ```python | |||
There was a problem hiding this comment.
Change this to ```py. This is a shortcut for ```python.
There was a problem hiding this comment.
As it's a test fixture I really don't see the point of changing this. If I touch it, maybe I can add a new block with py so the test ensure both are working?
There was a problem hiding this comment.
Yes, many people use py for shorthand anyway as it saves them some typing so I think adding that block anyway would be great.
There was a problem hiding this comment.
That's probably not really the responsibility of the readme_renderer tests to validate this, but it may not hurt to add it so we're also ensuring that this particular tag, probably the most used in this context, still work, so I added it.
tests/test_any.py
Outdated
| return | ||
| # Get our Markup | ||
| with io.open(src_filename, encoding='utf-8') as f: | ||
| markup = f.read() |
There was a problem hiding this comment.
Use 'open' that is built in instead of importing it just to open files.
There was a problem hiding this comment.
open from Python 2.7 had no encoding parameter, while we want this to work on pypi-legacy, we still need to keep Python 2 compatibility, also that's how tests are written for rst, so it's probably nice to write them the same way for consistency, and as I don't want to modify rst tests in a markdown branch, I think we should keep this that way. I'm not fluent in Python 2 / Python 3 compatible code, but as far as I searched it looks the right way to do it.
|
|
||
| def read(fn): | ||
| path = os.path.join(os.path.dirname(__file__), 'fixtures', 'markdown', fn) | ||
| with io.open(path, encoding='utf-8') as f: |
There was a problem hiding this comment.
Same, Python 2 compatibility, to give the encoding.
There was a problem hiding this comment.
Alright, so we can change it later when python 2 is decided to be fully dropped?
Or maybe I could figure out how to port the encoding parameter to the builtin open function for the next version of python 2.7 so then this can be changed.
There was a problem hiding this comment.
Yes, I'm OK to change it later when python2 compat is dropped.
Look like pypi-legacy has an indeterminate lifespan, but if we pin a legacy version of readme_renderer in pypi-lecacy requirements, we'll be able to drop python2 compatibility. For the moment, I think it's better to keep with python2 compatibility so pypi-legacy can at least get markdown.
There was a problem hiding this comment.
Yeah, or port python 2's open function to accept `encoding`` as an paramiter like how python 3 does. I would have to figure out a patch for python 2 then to allow such. (because I only use python 3 now)
There was a problem hiding this comment.
I'm not sure backporting the encoding parameter to Python 2's open is worth the effort since Python 2 is end of life. Let's just stick, like everyone else, to a simple and temporary "import io".
Also, it will be easier to port Python 2 using io.open than Python 2 using open, as Python 3's open is io.open.
There was a problem hiding this comment.
A comment here might be useful, mentioning that this a Py2 compatibility thing.
|
This seems fine to me in principle, barring one thing. Could you rename "any" to something else (like base)? Because this is a module; naming it to mask a built-in is really, not nice. |
|
@pradyunsg done. |
|
Thanks @JulienPalard! I think this needs @dstufft's review now, which I presume would happen after July 4th. :) |
|
Hi @JulienPalard, thanks for your work here. Since we now have a way for the user to explicitly describe the content-type of their README, being able to infer it isn't necessary. I really appreciate you taking a shot at it though! |
|
Hi @di, Nice job, a "Content-Type" metadata is clearly the right way to do it cleanly. Autodetection will always keep a few degree of errors. Yet I don't think the 100k packages currently already uploaded will upgrade anytime soon, so fallbacking on autodetection may still make sense when the content type is not provided. So in one hand, fallbacking to auto-detection when content type is missing fix all already-uploaded packages and permits to omit it when it's not necessary. On the other hand auto-detecting may have the counter-productive effect of keeping people away from the Content-Type metadata, leading sometimes to surprises (misdetection). I think I prefer not doing auto-detection (only relying on Content-Type defaulting to rst), but it's hard to keep those badly rendered READMEs online. |
On top of the work of @msabramo on #1, I added my mdorrst to properly dispatch to the right renderer.
This introduces
readme_renderer.any.render, dispatching itself toreadme_renderer.rst.render,readme_renderer.markdown.render, andreadme_renderer.txt.render.anymay not be the best choice as it conflicts with a builtin, but only an "import *" or an "import readme_renderer.any as any" can override the builtin, so I prefer the verbosity of the name, is someone has better in mind just tell me. We may also "from .any import render" in__init__.pyso other projects will simply use "readme_renderer.render(long_description)"./!\ This renderer in fact modifies the rendering of some existing README:
Before, we were always trying to render long_descriptions as rst, even if it was plaintext or HTML or whatever, we were fallbacking only if rst rendering was failing.
Now, this branch simply use the txt renderer if the content is "obviously only text". It may be what we want, it may not, (and I'm sure it's we want for half of them, and not what we want for the other half...).