Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

rustdoc: escape the deprecated and unstable reason text #38244

Merged
merged 4 commits into from
Jan 9, 2017

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Dec 8, 2016

Fix #38220.
Instead of the current output:

incorrect unescaped unstable reason in docs

display:

escaped unstable reason in docs

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

(rust_highfive has picked a reviewer for you, use r? to override)

@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 8, 2016

📌 Commit 4cfc1e3 has been approved by petrochenkov

Copy link
Member

@ollie27 ollie27 left a comment

Choose a reason for hiding this comment

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

These strings are run through a Markdown renderer after this and it doesn't make any sense to HTML escape something and then treat it as Markdown. I think the right thing to do here is replace the Markdown rendering with just escaping.

Also there is one other instance of this below these two that should probably be changed as well.

@estebank
Copy link
Contributor Author

estebank commented Dec 8, 2016

@petrochenkov
Copy link
Contributor

@bors r-
@bors delegate=ollie27

@bors
Copy link
Contributor

bors commented Dec 8, 2016

✌️ @ollie27 can now approve this pull request

@ollie27
Copy link
Member

ollie27 commented Dec 8, 2016

Actually we could leave it using Markdown and just add `s around Box<FnOnce> in the stability message like is done elsewhere.

@estebank
Copy link
Contributor Author

estebank commented Dec 8, 2016

@ollie27 I'm thinking that even if using Markdown syntax to mark code would be preferable here it should still look reasonable (as in escaped) even if the code author forgets to do so.

HTML Escaping the text of the unstable reasons and then passing Markdown syntax will only avoid people from using HTML anchors for links or HTML tags for formatting, little more. I don't think any part of the markdown syntax would be affected by the escaping in the first place :)

@ollie27
Copy link
Member

ollie27 commented Dec 9, 2016

Adding the quotes and escaping results in it being rendered as "will be deprecated if and when Box&lt;FnOnce&gt; becomes usable", so yes escaping first does break Markdown.

@estebank
Copy link
Contributor Author

estebank commented Dec 9, 2016

@ollie27 you're absolutely right.

@steveklabnik
Copy link
Member

/cc @rust-lang/tools , how do we want to move forward here?

@alexcrichton
Copy link
Member

Sounds like @ollie27's suggestion may be the least invasive?

`MarkdownHtml` structs escape HTML tags from its text.
@estebank
Copy link
Contributor Author

After reading through Hoedown's documentation, I have added a new MarkdownHtml struct that escapes HTML tags from the output, presenting the following output:

for reason "will be deprecated if and when `Box<FnOnce>` Box<FnOnce> becomes usable".

@estebank
Copy link
Contributor Author

estebank commented Dec 20, 2016

@ollie27 I feel that the patch is ready for merging. It changes this specific instance of the issue to use the code formatting, and avoids the issue going forward whenever somebody actually forgets to do so by escaping correctly by relying on Hoedown's feature flags. Only drawback is that you can't use HTML tags in these messages, but I feel that no one would be missing that feature for this field.

@ollie27
Copy link
Member

ollie27 commented Dec 22, 2016

I'm not sure about having the Markdown rendering act differently here to other places but as you've said it probably won't cause any problems in reality. We'll have to see what other people think.

Either way, if this change is accepted it will need tests.

@estebank estebank force-pushed the escape-reason-docs branch 2 times, most recently from 20a8320 to 129a385 Compare December 25, 2016 19:39
@bors
Copy link
Contributor

bors commented Dec 27, 2016

☔ The latest upstream changes (presumably #38329) made this pull request unmergeable. Please resolve the merge conflicts.

@steveklabnik
Copy link
Member

ping @ollie27 are you happy with this now?

@ollie27
Copy link
Member

ollie27 commented Jan 5, 2017

Yeah this is fine with me. Are people okay with no longer allowing raw HTML in depreciation and stability messages?

@nrc
Copy link
Member

nrc commented Jan 5, 2017

@ollie27 sgtm

@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2017

@ollie27 if you're happy with the PR, can you post "@bors r+" for @bors to automerge this change? :)

@ollie27
Copy link
Member

ollie27 commented Jan 9, 2017

Sure.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 9, 2017

📌 Commit e766c46 has been approved by ollie27

@bors
Copy link
Contributor

bors commented Jan 9, 2017

⌛ Testing commit e766c46 with merge 9749df5...

bors added a commit that referenced this pull request Jan 9, 2017
rustdoc: escape the deprecated and unstable reason text

Fix #38220.
Instead of the [current output](https://doc.rust-lang.org/std/boxed/trait.FnBox.html):

<img width="967" alt="incorrect unescaped unstable reason in docs" src="https://cloud.githubusercontent.com/assets/1606434/21021898/73121d42-bd2f-11e6-8076-8a5127dbc010.png">

display:

<img width="979" alt="escaped unstable reason in docs" src="https://cloud.githubusercontent.com/assets/1606434/21021876/52eb0f88-bd2f-11e6-9088-58bdc7d92328.png">
@bors
Copy link
Contributor

bors commented Jan 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: ollie27
Pushing 9749df5 to master...

@bors bors merged commit e766c46 into rust-lang:master Jan 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants