Summary

Always pass text to markdown()

Review Request #10699 — Created Sept. 7, 2019 and discarded Sept. 10, 2019, 1:24 p.m.

Information

Owner

brennie

Repository

Djblets

Branch

master

Bugs

Depends On

Blocks

10701

Reviewers

Groups

djblets

People

Description

Previously, we were passing a binary file to markdown.markdownFromFile in

render_markdown_from_file, but this won't fly in Python 3. It expects

a file opened with an encoding. However, Django does not let you specify

an encoding when opening a file from storage, so that solution is out.

We now just read the entire file into memory, decode it from UTF-8, and

then pass that to markdown.markdown(), which returns text directly.

Testing Done

With this patch applied, text file attachments render correctly
in Review Board on Python 2.7, 3.5, 3.6, and 3.7.

Commits

Summary	ID	Author
Always pass text to markdown() Previously, we were passing a binary file to `markdown.markdownFromFile` in `render_markdown_from_file`, but this won't fly in Python 3. It expects a file opened with an encoding. However, Django does not let you specify an encoding when opening a file from storage, so that solution is out. We now just read the entire file into memory, decode it from UTF-8, and then pass that to `markdown.markdown()`, which returns text directly. Testing done: With this patch applied, text file attachments render correctly in Review Board on Python 2.7, 3.5, 3.6, and 3.7.	461754fc77a94d7e3573e58eb4f3c6ce828efeb4	Barret Rennie

Issues

Description	From	Last Updated
F401 'markdown.Markdown' imported but unused	reviewbot	Sept. 7, 2019, 12:57 p.m.
What's the error you hit? I tried passing in a FileAttachment.file into markdownFromFile as input and it handled it right. …	chipx86	Sept. 7, 2019, 1:11 p.m.
Can you remove the blank lines at the end of the file?	chipx86	Sept. 7, 2019, 1:11 p.m.

flake8 failed.

JSHint passed.

flake8

djblets/markdown/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'markdown.Markdown' imported but unused
```

Commits:

	Summary	ID	Author
	Always pass text to markdown() Previously, we were passing a binary file to `markdown.markdownFromFile` in `render_markdown_from_file`, but this won't fly in Python 3. It expects a file opened with an encoding. However, Django does not let you specify an encoding when opening a file from storage, so that solution is out. We now just read the entire file into memory, decode it from UTF-8, and then pass that to `markdown.markdown()`, which returns text directly. Testing done: With this patch applied, text file attachments render correctly in Review Board on Python 2.7, 3.5, 3.6, and 3.7.	ce9e515d6089f5ea539a86901cd99fea2013bc55	Barret Rennie
	Always pass text to markdown() Previously, we were passing a binary file to `markdown.markdownFromFile` in `render_markdown_from_file`, but this won't fly in Python 3. It expects a file opened with an encoding. However, Django does not let you specify an encoding when opening a file from storage, so that solution is out. We now just read the entire file into memory, decode it from UTF-8, and then pass that to `markdown.markdown()`, which returns text directly. Testing done: With this patch applied, text file attachments render correctly in Review Board on Python 2.7, 3.5, 3.6, and 3.7.	461754fc77a94d7e3573e58eb4f3c6ce828efeb4	Barret Rennie

Diff:

Revision 2 (+14 -12)

Show changes

djblets/markdown/__init__.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

djblets/markdown/__init__.py (Diff revision 2)

An issue was opened. Show all issues

What's the error you hit? I tried passing in a FileAttachment.file into markdownFromFile as input and it handled it right.

Looking at the code, it attempts to do a codecs.getreader()(input) if you pass it a file handle, and it only needs .read() and .close(), both of which Django's File supports.

In my test, I was able to verify that File.read() was giving bytes normally, but passing it into markdownFromFile rendered unicode fine. This on Python 3.x.

djblets/markdown/__init__.py (Diff revision 2)
An issue was opened. Show all issues
```
Can you remove the blank lines at the end of the file?
```

Status:: Discarded
Change Summary:: Discarding based on conversation with Christian.