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

Use double quote for rustdoc html #76096

Merged
merged 2 commits into from
Oct 17, 2020
Merged

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Aug 30, 2020

r? @GuillaumeGomez

Feels scary without escaping stuff when I looked at the code, probably susceptible to XSS.
Follow up of #75842

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@ollie27
Copy link
Member

ollie27 commented Aug 30, 2020

I would prefer we standardize on using double quote everywhere because that's what pulldown-cmark uses.

@ollie27 ollie27 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 30, 2020
@pickfire
Copy link
Contributor Author

pickfire commented Aug 30, 2020

@ollie27 But that require escaping everywhere, or maybe we could do r#"xxx"# to reduce escapes?

@ollie27
Copy link
Member

ollie27 commented Aug 30, 2020

But that require escaping everywhere, or maybe we could do r#"xxx"# to reduce escapes?

I don't think that's too big of a deal. Unfortunately r#"xxx"# doesn't work well for multi line strings which is probably why it's not used currently.

@pickfire pickfire changed the title Use single quote for rustdoc html Use double quote for rustdoc html Aug 31, 2020
@pickfire
Copy link
Contributor Author

pickfire commented Aug 31, 2020

Double quotes usage is less than single quote right now, 85 vs 124 lines.

@bors
Copy link
Contributor

bors commented Sep 1, 2020

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

@jyn514 jyn514 added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Sep 3, 2020
@JohnCSimon JohnCSimon 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 Sep 21, 2020
@JohnCSimon
Copy link
Member

Ping from triage
@pickfire Can you please address the merge conflict and failing build? Thank you.

@Dylan-DPC-zz
Copy link

@pickfire any updates on this pr?

@pickfire
Copy link
Contributor Author

pickfire commented Sep 22, 2020

I was waiting for reviewer to say if we should proceed with this, if it is good then I will go and resolve the conflicts and fix the build, otherwise can just close it. Thanks for the ping.

@crlf0710 crlf0710 added I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 8, 2020
@crlf0710
Copy link
Member

crlf0710 commented Oct 8, 2020

Triage: It seems this PR needs decision here. @GuillaumeGomez @jyn514

@jyn514
Copy link
Member

jyn514 commented Oct 8, 2020

@pickfire if you use double quotes like suggested by ollie (#76096 (comment)) and revert the change to resource_suffix then I think this good to go.

@jyn514 jyn514 removed I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 8, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 8, 2020

Oh wait, you already did that 😆 this looks fine after a rebase then.

@pickfire pickfire force-pushed the rustdoc-quote branch 2 times, most recently from 0ee0d7d to 29519b7 Compare October 10, 2020 02:38
@bors
Copy link
Contributor

bors commented Oct 14, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@pickfire
Copy link
Contributor Author

@jyn514 I rebased but now there is a merge conflict again.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

@pickfire please ping me after you rebase, I don't know to review unless you say something. Sorry you have to rebase again.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

@bors p=1

This PR bitrots quickly.

@pickfire
Copy link
Contributor Author

@jyn514 ping

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

@pickfire you need to run x.py fmt:

 Diff in /checkout/src/librustdoc/html/render/mod.rs at line 87:
 
 crate fn ensure_trailing_slash(v: &str) -> impl fmt::Display + '_ {
     crate::html::format::display_fn(move |f| {
-        if !v.ends_with('/') && !v.is_empty() {
-            write!(f, "{}/", v)
-        } else {
-            write!(f, "{}", v)
-        }
+        if !v.ends_with('/') && !v.is_empty() { write!(f, "{}/", v) } else { write!(f, "{}", v) }
     })
 }
 
Diff in /checkout/src/librustdoc/html/render/mod.rs at line 1870:
 }
 
 fn document_non_exhaustive_header(item: &clean::Item) -> &str {
-    if item.is_non_exhaustive() {
-        " (Non-exhaustive)"
-    } else {
-        ""
-    }
+    if item.is_non_exhaustive() { " (Non-exhaustive)" } else { "" }
 }
 
 fn document_non_exhaustive(w: &mut Buffer, item: &clean::Item) {
Diff in /checkout/src/librustdoc/html/render/mod.rs at line 4144:
                                 if is_negative_impl { "!" } else { "" },
                                 out
                             );
-                            if links.insert(generated.clone()) {
-                                Some(generated)
-                            } else {
-                                None
-                            }
+                            if links.insert(generated.clone()) { Some(generated) } else { None }
                         } else {
                             None
                         }

@pickfire
Copy link
Contributor Author

I won't be running ./x.py fmt, I probably need to run that command when I sleep (I am sleeping soon anyway) and get the result when I wake up. I just fixed it manually according to the CI output.

@jyn514
Copy link
Member

jyn514 commented Oct 15, 2020

@pickfire x.py fmt only takes a few seconds to run (at most a few minutes if you have to compile bootstrap first). But if it works, it works.

@pickfire
Copy link
Contributor Author

I already took a few minutes to compile bootstrap. Oh no, merge conflicts! T_T

@bors
Copy link
Contributor

bors commented Oct 16, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@pickfire
Copy link
Contributor Author

pickfire commented Oct 16, 2020

@jyn514 pong

@jyn514
Copy link
Member

jyn514 commented Oct 16, 2020

@bors delegate=pickfire

Please wait to approve until CI passes, and use r=jyn514 instead of r+. Thanks for sticking with this, I know merge conflicts are painful.

@bors
Copy link
Contributor

bors commented Oct 16, 2020

✌️ @pickfire can now approve this pull request

@jyn514
Copy link
Member

jyn514 commented Oct 16, 2020

---- [rustdoc] rustdoc/index-page.rs stdout ----
 10: @has check failed
	`XPATH PATTERN` did not match
	// @has - '//ul[@class="crate mod"]//a[@href="all_item_types/index.html"]' 'all_item_types'

Maybe you made a typo somewhere?

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
@pickfire
Copy link
Contributor Author

@ollie27 Thanks

r=jyn514

@pickfire
Copy link
Contributor Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Oct 17, 2020

📌 Commit e96ca1b has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 17, 2020
@bors
Copy link
Contributor

bors commented Oct 17, 2020

⌛ Testing commit e96ca1b with merge 03687f8...

@bors
Copy link
Contributor

bors commented Oct 17, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 03687f8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2020
@bors bors merged commit 03687f8 into rust-lang:master Oct 17, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 17, 2020
@pickfire pickfire deleted the rustdoc-quote branch October 17, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants