-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
replace serialize with serde in rustdoc #62359
Conversation
src/librustdoc/html/render.rs
Outdated
#[derive(Serialize)] | ||
struct CrateData<'a> { | ||
doc: String, | ||
#[serde(rename = "i")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes it so much simpler to understand! \o/
If the generated index is the same as before, then it's all good for me. I'd just prefer if @QuietMisdreavus took a look to confirm and then here we go. Thanks a lot! |
Hey! This is a ping from triage, we would like to know if you @QuietMisdreavus could give us a few minutes to share your thoughts on it. Thanks. |
&self.desc, | ||
self.parent_idx, | ||
&self.search_type, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a tuple the right construct for this? Do serialize_tuple
and serialize_seq
come out the same when run through serde_json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will be serialized the same way. This way, it saves the intermediate step of serializing everything to Value
to make a Vec<Value>
.
☔ The latest upstream changes (presumably #63207) made this pull request unmergeable. Please resolve the merge conflicts. |
Ping from triage, @euclio please resolve the merge conflicts. Thanks |
2603f32
to
698a239
Compare
Rebased. |
Ping from triage Thank you! |
Looks good to me. Just one question: is there any difference in term of performance? |
☔ The latest upstream changes (presumably #63579) made this pull request unmergeable. Please resolve the merge conflicts. |
698a239
to
972f0e0
Compare
I'm going to approve this per #62359 (comment) (and I've done my own review-ish, without trying to get into details of one to one compat). @bors r+ We're still fairly early in the release cycle as well so if something comes up we can revert, and since I'll be touching some of this code in some of my PRs I'd like to avoid conflicts. |
📌 Commit 972f0e07334917ea4690185a07b50c7ed557c5ea has been approved by |
⌛ Testing commit 972f0e07334917ea4690185a07b50c7ed557c5ea with merge 1675253b27b29808f17637a617b0edfe3e5374d5... |
💔 Test failed - checks-azure |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@euclio you need to address the failing tests |
retrying it |
@euclio any updates on this? |
@Dylan-DPC Needs a retry, looks like the last one didn't happen. |
☔ The latest upstream changes (presumably #66828) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors retry |
b87dd82
to
d2fe425
Compare
☔ The latest upstream changes (presumably #67198) made this pull request unmergeable. Please resolve the merge conflicts. |
d2fe425
to
94630d4
Compare
Rebased. |
@bors r+ rollup=never |
📌 Commit 94630d4 has been approved by |
replace serialize with serde in rustdoc This is a slightly less aggressive version of #61028. r? @GuillaumeGomez
☀️ Test successful - checks-azure |
This is a slightly less aggressive version of #61028.
r? @GuillaumeGomez