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.

Sign up for GitHub

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

Jump to bottom

Migrate item_trait_alias to Askama #112030

Merged
merged 1 commit into from
Jun 2, 2023
Merged

Migrate item_trait_alias to Askama #112030

merged 1 commit into from
Jun 2, 2023

Conversation

sladyn98
Copy link
Contributor

This PR migrates item_trait_alias to Askama

Refers #108868

@rustbot
Copy link
Collaborator

rustbot commented May 28, 2023

r? @GuillaumeGomez

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 28, 2023
@GuillaumeGomez
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit 9520f591368f27e39637e7c566d693c7eeaab6e3 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2023
GuillaumeGomez
GuillaumeGomez reviewed May 29, 2023
View reviewed changes
src/librustdoc/html/render/print_item.rs Outdated
t: &clean::TraitAlias,
) {
let mut buffer = Buffer::new();
wrap_item(&mut buffer, |buffer| {
Copy link
Member

Choose a reason for hiding this comment

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

Something I just thought about: is it actually necessary to pass Buffer as argument to wrap_item? If we're forced to, would it make sense to have another wrap_item_write which would take impl fmt::Write instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could actually write another wrap_item_write that would take in impl fmt::Write and it would actually reduce the overhead of creating a buffer every single time.

fn wrap_item_write<F>(w: &mut impl fmt::Write, f: F)
where
    F: FnOnce(&mut impl fmt::Write),
{
    write!(w, r#"<pre class="rust item-decl"><code>"#).unwrap();
    f(w);
    write!(w, "</code></pre>").unwrap();
}

Copy link
Member

Choose a reason for hiding this comment

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

Then please do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets do it in this PR itself and then we can merge the rest, or do you want me to create a new PR to implement that function

Copy link
Member

Choose a reason for hiding this comment

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

You can do it in this PR directly.

@GuillaumeGomez
Copy link
Member

Waiting for an answer to my comment.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 29, 2023
This was referenced May 30, 2023
Merged
Closed
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 1, 2023

@GuillaumeGomez Implemented the wrap_item_write, looks like we can get rid of the buffer now. Let me know if you have any suggestions

GuillaumeGomez
GuillaumeGomez reviewed Jun 1, 2023
View reviewed changes
src/librustdoc/html/render/print_item.rs Outdated
@@ -1675,6 +1681,16 @@ where
w.write_str("</code></pre>");
}

fn wrap_item_write<W, F>(w: &mut W, f: F)
Copy link
Member

Choose a reason for hiding this comment

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

Buffer actually already implements the fmt::Write trait. Wouldn't it be simpler to update wrap_item instead of creating a new function?

Copy link
Contributor Author

@sladyn98 sladyn98 Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that makes perfect sense, updated the function : )

@GuillaumeGomez
Copy link
Member

Looks like it's all good, thanks for working on this! Once CI pass, r=me

@sladyn98
Migrate to Askama
780719b 
Implemented wrap_item_write

Update wrap_item
@sladyn98
Copy link
Contributor Author

sladyn98 commented Jun 1, 2023 •
edited
Loading

r?@GuillaumeGomez

@rustbot
Copy link
Collaborator

rustbot commented Jun 1, 2023

Could not assign reviewer from: GuillaumeGomez.
User(s) GuillaumeGomez are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

@GuillaumeGomez
Copy link
Member

No need to re-assign me every time, I get notifications for every event on a PR I'm subscribed to. ;)

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 1, 2023

📌 Commit 780719b has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 Jun 1, 2023
@compiler-errors compiler-errors mentioned this pull request Jun 2, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 2, 2023
@bors
Auto merge of rust-lang#112198 - compiler-errors:rollup-o2xe4of, r=co…
8ebf042 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111670 (Require that const param tys implement `ConstParamTy`)
 - rust-lang#111914 (CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi…)
 - rust-lang#112030 (Migrate `item_trait_alias` to Askama)
 - rust-lang#112150 (Support 128-bit atomics on all x86_64 Apple targets)
 - rust-lang#112174 (Fix broken link)
 - rust-lang#112190 (Improve comments on `TyCtxt` and `GlobalCtxt`.)
 - rust-lang#112193 (Check tuple elements are `Sized` in `offset_of`)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ceb283 into rust-lang:master Jun 2, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 2, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
@bors
Auto merge of rust-lang#112198 - compiler-errors:rollup-o2xe4of, r=co…
427a453 
…mpiler-errors

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111670 (Require that const param tys implement `ConstParamTy`)
 - rust-lang#111914 (CFI: Fix cfi with async: transform_ty: unexpected GeneratorWitness(Bi…)
 - rust-lang#112030 (Migrate `item_trait_alias` to Askama)
 - rust-lang#112150 (Support 128-bit atomics on all x86_64 Apple targets)
 - rust-lang#112174 (Fix broken link)
 - rust-lang#112190 (Improve comments on `TyCtxt` and `GlobalCtxt`.)
 - rust-lang#112193 (Check tuple elements are `Sized` in `offset_of`)

Failed merges:

 - rust-lang#112071 (Group rfcs tests)

r? `@ghost`
`@rustbot` modify labels: rollup
@clubby789 clubby789 mentioned this pull request Jun 10, 2024
Open
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez Awaiting requested review from GuillaumeGomez

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
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
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sladyn98 @rustbot @GuillaumeGomez @bors