Skip to content

Md warnings #46247

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Dec 8, 2017
Merged

Md warnings #46247

merged 2 commits into from
Dec 8, 2017

Conversation

GuillaumeGomez
Copy link
Member

let orig_used_id_map = USED_ID_MAP.with(|map| map.borrow().clone());
let hoedown_output = format!("{}", MarkdownWithToc(text, RenderType::Hoedown));
USED_ID_MAP.with(|map| *map.borrow_mut() = orig_used_id_map);
let pulldown_output = format!("{}", MarkdownWithToc(text, RenderType::Pulldown));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can somehow combine this chunk of code with the one below and with https://github.com/GuillaumeGomez/rust/blob/92c38d31e4f3797517d1a33619eb2628758cf464/src/librustdoc/html/render.rs#L1838-L1843 so we don't have three copies of the "render this markdown twice" code? (I'm thinking some kind of function that takes a RenderType and returns a string so it can also be used right here?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about it as well but every solution seems more like a boilerplate than a real solution. I'll give it a deeper look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my thinking is something like this, in html/render.rs so it can be used alongside USED_ID_MAP:

fn render_text<F: FnMut(RenderType) -> String>(render: F) -> (String, String) {
    // (original comment about saving USED_ID_MAP)
    let orig_used_id_map = USED_ID_MAP.with(|map| map.borrow().clone());
    let hoedown_output = render(text, RenderType::Hoedown);
    USED_ID_MAP.with(|map| *map.borrow_mut() = orig_used_id_map);
    let pulldown_output = render(text, RenderType::Pulldown);
    (hoedown_output, pulldown_output)
}

...and then it could be called like this:

render_text(|type| format!("{}", Markdown(text, type)));
render_text(|type| format!("{}", MarkdownWithToc(text, type)));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally seems like a boilerplate haha. I'll give it a try. ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how refactoring this to combine the code would be called "boilerplate", but if you think it would be too much effort i can just drop it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you export render_text and use it here? (That's why i brought it up in the first place. >_>)

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2017
@shepmaster shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2017
@shepmaster
Copy link
Member

Checking in from triage — it looks like it's up to @GuillaumeGomez to make some suggested changes, yes?

@QuietMisdreavus
Copy link
Member

That or some kind of field report for having tried them out and rejecting them. The rest of the PR is good.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 2, 2017

I need to apply @QuietMisdreavus' suggestions. I didn't have time yet.

@GuillaumeGomez
Copy link
Member Author

@QuietMisdreavus: Updated. The new code looks way better. Great suggestion!

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 6, 2017
@QuietMisdreavus
Copy link
Member

r=me pending travis

@GuillaumeGomez
Copy link
Member Author

@bors: r=QuietMisdreqvus

@bors
Copy link
Collaborator

bors commented Dec 8, 2017

📌 Commit eb84f42 has been approved by QuietMisdreqvus

@bors
Copy link
Collaborator

bors commented Dec 8, 2017

⌛ Testing commit eb84f42 with merge ab79caa...

bors added a commit that referenced this pull request Dec 8, 2017
@bors
Copy link
Collaborator

bors commented Dec 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreqvus
Pushing ab79caa to master...

@bors bors merged commit eb84f42 into rust-lang:master Dec 8, 2017
@GuillaumeGomez GuillaumeGomez deleted the md-warnings branch December 9, 2017 11:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants