Record readme filename; use to determine whether to render as Markdown or not #1134

kivikakk · 2017-10-17T06:56:15Z

Fixes #995. Depends on rust-lang/cargo#4633 to have the desired effect, but won't break in the absence of that PR; we'll just assume the readme is called "README.md" if the client doesn't report one.

Small CSS fix included to wrap extra long lines.

Comparison of render of https://crates.io/crates/relm's README:

Before	After

kureuil

Thank you so much for this PR @kivikakk !

I just have a few remarks, otherwise this seems great to me :)

kureuil · 2017-10-17T08:33:08Z

src/render.rs

@@ -141,11 +142,31 @@ impl<'a> Default for MarkdownRenderer<'a> {
 /// let text = "[Rust](https://rust-lang.org/) is an awesome *systems programming* language!";
 /// let rendered = markdown_to_html(text)?;
 /// ```
-pub fn markdown_to_html(text: &str) -> CargoResult<String> {
+fn markdown_to_html(text: &str) -> CargoResult<String> {


If we drop the pub modifier on markdown_to_html, we probably want to drop it for struct MarkdownRenderer too ?

kureuil · 2017-10-17T08:35:34Z

src/render.rs

    let renderer = MarkdownRenderer::new();
    renderer.to_html(text)
 }

+static MARKDOWN_EXTENSIONS: [&'static str; 7] = [


Even though this is not public & pretty self-explanatory, we might want to add a small doc comment to this constant.

kureuil · 2017-10-17T08:43:00Z

src/render.rs

+    ".mkdown",
+];
+
+pub fn readme_to_html(text: &str, filename: &str) -> CargoResult<String> {


Could you add a doc comment on this function ?

Whoops, forgot!

kureuil · 2017-10-17T08:53:36Z

src/render.rs

+        }
+    }
+
+    Ok(encode_minimal(text).replace("\n", "<br>\n"))


Why are we pulling htmlescape as a new dependency ? Isn't ammonia already doing a similar job ?

htmlescape is replacing characters with their entities in order to basically turn text into HTML that represents/renders as the same text; ammonia is parsing HTML itself and then sanitizing that DOM. They're pretty different (and unfortunately, I looked and neither ammonia nor any of its dependencies, like html5ever, can do the job of htmlescape simply.)

kureuil · 2017-10-17T08:58:45Z

src/krate/mod.rs

@@ -65,6 +65,7 @@ pub struct Crate {
    pub homepage: Option<String>,
    pub documentation: Option<String>,
    pub readme: Option<String>,
+    pub readme_file: Option<String>,


I see that we're now storing the README filename alongside its content, though it doesn't seem to be used anywhere ? I'm not opposed to storing it but I'm not sure to see how useful it is to store it in the database.

You're right, we don't strictly need to store it. I was thinking we might need it in render-readmes, but that pulls the README filename from the manifest directly. However, I feel like storing this as well might be important for things like #1025/#1019?

A more general argument is that, if there's no need to store the README filename in the database, then there's no need to store the README content in the database, because we can't properly make sense of the latter without the former.

That's moreorless the motivation of this PR in a nutshell: we don't know how to interpret the README contents without knowing its filename. So if we store the content, we should store the filename too.

Yeah, you're totally right about needing to store the filename alongside the README content, my bad.

However, maybe we can improve a little here and instead of storing the README filename as transmitted, how about storing the normalized README type ? i.e only markdown for now. This would also allow us to use an ENUM instead of a VARCHAR field in the database and simplify its usage.

I like the idea of doing this, but I'm worried about the loss of fidelity. e.g. say we do so, and currently store 'markdown' or NULL for everything. Later we add AsciiDoc or org-mode support — it'd be great to go back and rerender the newly supported readmes, but to do so we'd have to reach back inside each crate and retrieve the readme filename from the manifest again.

I think storing the filename directly allows us more flexibility later on in how we want to treat certain things, especially in the face of changing/new standards and formats; this is how we approach file classification at GitHub, for example, and it works well for us.

but to do so we'd have to reach back inside each crate and retrieve the readme filename from the manifest again.

In any case, we probably will have to do it. The crates table only stores the README of the last published version of a given crate, so if we add org-mode or AsciiDoc support to crates.io we'd still have to reach back inside each crate in order to render it. Anyway, you're right about your approach being more flexible so let's keep it this way :)

sgrif · 2017-10-18T21:03:39Z

I think we should still assume that README with no extension is markdown here. Other than that this looks fine.

…ename

kivikakk · 2017-10-19T03:25:15Z

@sgrif That works for me! Change made.

…ename

carols10cents · 2017-10-25T18:30:14Z

Looks great and works great with the cargo change, and the current behavior still works with stable cargo! Just merged in upstream to resolve the merge conflicts :)

bors: r+

1134: Record readme filename; use to determine whether to render as Markdown or not r=carols10cents Fixes #995. Depends on rust-lang/cargo#4633 to have the desired effect, but won't break in the absence of that PR; we'll just assume the readme is called `"README.md"` if the client doesn't report one. Small CSS fix included to wrap extra long lines. Comparison of render of https://crates.io/crates/relm's README: | Before | After | | :-: | :-: | | ![screen shot 2017-10-17 at 5 51 50 pm](https://user-images.githubusercontent.com/1915/31650690-42018c7a-b364-11e7-95f8-7e1b8e8d56a0.png) | ![screen shot 2017-10-17 at 5 51 51 pm](https://user-images.githubusercontent.com/1915/31650698-44d16588-b364-11e7-92cf-1c3201c0e60e.png) |

bors-voyager · 2017-10-25T19:11:55Z

Build succeeded

continuous-integration/travis-ci/push

kivikakk added 4 commits October 17, 2017 16:57

Record readme filename from cargo transmit

dca3299

Render non-Markdown READMEs as plain text

12c4762

Don't let this exit the container

633a214

clippy!

0b87650

kureuil suggested changes Oct 17, 2017

View reviewed changes

Cleanup based on @kureuil's comments

c00faea

kivikakk added 2 commits October 19, 2017 14:19

No extension = assume Markdown.

0dde304

Merge remote-tracking branch 'upstream/master' into record-readme-fil…

80129d1

…ename

Merge remote-tracking branch 'upstream/master' into record-readme-fil…

931427f

…ename

bors-voyager bot merged commit 931427f into rust-lang:master Oct 25, 2017

kivikakk deleted the record-readme-filename branch October 26, 2017 00:14

Turbo87 mentioned this pull request Feb 14, 2021

Render non-markdown READMEs #1223

Closed

Record readme filename; use to determine whether to render as Markdown or not #1134

Record readme filename; use to determine whether to render as Markdown or not #1134

Uh oh!

Conversation

kivikakk commented Oct 17, 2017

Uh oh!

kureuil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kivikakk Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgrif commented Oct 18, 2017

Uh oh!

kivikakk commented Oct 19, 2017

Uh oh!

carols10cents commented Oct 25, 2017

Uh oh!

bors-voyager bot commented Oct 25, 2017

Build succeeded

Uh oh!

Uh oh!

kivikakk Oct 17, 2017 •

edited

Loading