[rustdoc] Remove UrlFragment::render method to unify clean::types::links and anchor #149028
Conversation
rustbot added S-waiting-on-review 
| I think we should block this on #149026 just to be safe. |
There was a problem hiding this comment.
Looks pretty good, just a few nits.
I am somewhat tempted to say in the future we should also unify the impl block section naming into the same function so that this same logic can be used for sidebar generation, but I'm not sure if that would be better than the status quo or not.
I actually tested with this one but I don't mind waiting. 😉 |
GuillaumeGomez force-pushed the unify-anchor branch 2 times, most recently from 3ad2a47 to 772c662 Compare | Applied suggestions. |
GuillaumeGomez force-pushed the unify-anchor branch from 772c662 to a5f972e Compare 
| This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
| Rebased on #149026. |
| Replaced the returned value of |
src/librustdoc/html/format.rs Outdated
| }; | ||
| let s; | ||
| let kind = if tcx.def_kind(parent_def_id) == DefKind::Variant { | ||
| s = format!("variant.{}.field", tcx.item_name(parent_def_id).as_str()); |
There was a problem hiding this comment.
now that we're using from_fn, we shouldn't need this intermediate allocation either, it should be possible to rewrite this branch just as a series of calls to write!.
GuillaumeGomez force-pushed the unify-anchor branch from f506f39 to a5e8eda Compare | Applied suggestion. |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez force-pushed the unify-anchor branch from a5e8eda to ae8dc3b Compare | Added back the |
| /// becoming a `TyMethod`). | ||
| pub(crate) fn from_def_kind(kind: DefKind, parent_kind: Option<DefKind>) -> Self { | ||
| match kind { | ||
| pub(crate) fn from_def_id(def_id: DefId, tcx: TyCtxt<'_>) -> Self { |
There was a problem hiding this comment.
now that i think about it, wouldn't of_def_id be a more descriptive name?
There was a problem hiding this comment.
Not exactly seems it involves a type conversion, not a direct translation.
src/librustdoc/html/format.rs Outdated
| } else { | ||
| String::new() | ||
| }; | ||
| f.write_char('#').unwrap(); |
There was a problem hiding this comment.
why are we using unwrap here instead of ?, the from_fn closure returns a Result containing fmt::Error.
in fact, the last write! of each branch does cascade it's error, so we should do the same with the others.
the unwrap in the other function is fine because there we have a concrete type of String.
There was a problem hiding this comment.
Arf indeed. Got so used to the unwrap everywhere that I simply used it here without second thought. Gonna check on a separate PRs to do the same change in the other from_fn calls.
GuillaumeGomez force-pushed the unify-anchor branch from ae8dc3b to 8f76773 Compare | Updated! |
| just realized if you don't give a top-level comment along with your review, rustbot doesn't have anywhere to put the "view changes since this review" link. |
| You can click on the |
| @bors r+ |
bors added S-waiting-on-bors Rollup of 7 pull requests Successful merges: - #146978 (Emit error when using path-segment keyword as cfg pred) - #148719 (Allow unnormalized types in drop elaboration) - #148795 (add `rust.rustflags` and per target `rustflags` options to `bootstrap.toml`) - #149028 ([rustdoc] Remove `UrlFragment::render` method to unify `clean::types::links` and `anchor`) - #149043 ( rustdoc-json: add rlib path to ExternalCrate to enable robust crate resolution) - #149098 (Fix error message for calling a non-tuple struct) - #149151 (Add test for importing path-segment keyword) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #149028 - GuillaumeGomez:unify-anchor, r=lolbinarycat [rustdoc] Remove `UrlFragment::render` method to unify `clean::types::links` and `anchor` Fixes #148648. First part of #148547. The last part will be about handle `AssocItemLink` differently (either remove it or change how we do it). r? `@lolbinarycat`
…amofek,lolbinarycat [rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls Following our discussion in rust-lang#149028 (comment), this PR makes more function return `fmt::Result`, allowing to use `?` a lot more, and also reducing number of `.unwrap()` calls. r? `@lolbinarycat`
Rollup merge of #149208 - GuillaumeGomez:less-unwraps, r=yotamofek,lolbinarycat [rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls Following our discussion in #149028 (comment), this PR makes more function return `fmt::Result`, allowing to use `?` a lot more, and also reducing number of `.unwrap()` calls. r? `@lolbinarycat`
…lbinarycat [rustdoc] Make more functions return `fmt::Result` and reduce number of `.unwrap()` calls Following our discussion in rust-lang/rust#149028 (comment), this PR makes more function return `fmt::Result`, allowing to use `?` a lot more, and also reducing number of `.unwrap()` calls. r? `@lolbinarycat`
5 participants
Fixes #148648.
First part of #148547.
The last part will be about handle
AssocItemLinkdifferently (either remove it or change how we do it).r? @lolbinarycat