Skip to content

rustdoc_json: more conversion cleanups #142896

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jun 23, 2025

A follow-up to #142747.

r? @aDotInTheVoid

No point computing these when they're only needed for one or two of the
match arms.
For consistency with most other types.
@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend 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 Jun 23, 2025
@nnethercote nnethercote changed the title Json conversion cleanups 2 rustdoc_json: more conversion cleanups Jun 23, 2025
@aDotInTheVoid
Copy link
Member

Re-rolling, as I have very limited review capacity at the moment, and this isn't particularly impactful/is more of a style thing that needing specific expertise.

r? rustdoc

@rustbot rustbot assigned notriddle and unassigned aDotInTheVoid Jul 7, 2025
@nnethercote
Copy link
Contributor Author

It has been a month since this was filed and two weeks since it was reassigned. Let's try another reviewer for this small change.

r? @GuillaumeGomez

@rustbot rustbot assigned GuillaumeGomez and unassigned notriddle Jul 22, 2025
@GuillaumeGomez
Copy link
Member

Interestingly enough, this is doing the opposite of what we did in the rest of rustdoc: replacing functions with trait implementations. We found it too complicated to keep track of the contexts/types when everything is just trait method calls and instead replaced that with plain function calls, making it easier to understand what was happening and also making it easier to be able to find the conversion code based on the function's name.

Maybe some extra insight on why we should finish this cleanup? #142747 was already merged after all but I'm not super enthusiast about this.

@nnethercote
Copy link
Contributor Author

It was a mix of trait methods and functions before. That seemed sub-optimal. The methods (a) were used more than functions, and (b) produce more concise and uniform code at the use points, so I converted a bunch of the functions to methods for consistency. But I won't fight for this to go in if there is opposition, it's not that important.

@GuillaumeGomez
Copy link
Member

I'll let @aDotInTheVoid have the final word here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants