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

Reword trait-object compatibility in rustdoc #126554

Closed
wants to merge 1 commit into from
Closed

Reword trait-object compatibility in rustdoc #126554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/librustdoc/html/render/print_item.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,13 +954,13 @@ fn item_trait(w: &mut Buffer, cx: &mut Context<'_>, it: &clean::Item, t: &clean:
if !t.is_object_safe(cx.tcx()) {
write_section_heading(
w,
"Object Safety",
"Trait-Object Safety",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Trait-Object Safety",
"Dyn-Compatibility",

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Trait-Object Safety",
"Dyn Compatibility",

"object-safety",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"object-safety",
"dyn-compatibility"

might need to update other references of this ID

Copy link
Contributor

@traviscross traviscross Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As a nit, one would probably say that "a trait is dyn compatible" (but one might "have a dyn-compatible trait"), in the same way that we're more likely to hyphenate "a forward-compatible change" than "a change that is forward compatible".

Similarly, I'd probably say "dyn compatibility" unhyphenated, in the same way "forward compatibility" most often is.

Copy link
Member

@fmease fmease Oct 3, 2024 •
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 sense (attributive vs. predicative usage). I went for unconditional hyphenation here and in my other "object safe → dyn compatible" PRs for legibility (well, that's slightly subjective I guess).

Apart from personal preference, my argument goes as follows: If you skim source code comments and documentation, a "stray" dyn has a good chance of tripping up your brain (well, that's my assumption) especially since it's jargon, not everyday language and since brains are prone to overlooking small words.

Anyway, I'm fine with conditional hyphenation here as you've suggested (since it probably looks better in the rendered HTML and my "source code skimming" argument doesn't really apply).

Copy link
Member

Choose a reason for hiding this comment

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

Tho note that this specific string should stay hyphenated for unrelated reasons – it represents the element ID.

None,
&format!(
"<div class=\"object-safety-info\">This trait is <b>not</b> \
"<div class=\"object-safety-info\">This trait is <strong>not</strong> compatible with \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"<div class=\"object-safety-info\">This trait is <strong>not</strong> compatible with \
"<div class=\"dyn-compat-info\">This trait is <strong>not</strong> compatible with \

and updating its references

<a href=\"{base}/reference/items/traits.html#object-safety\">\
Copy link
Member

@fmease fmease Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You should leave a FIXME(dyn_compat_renaming): Update the URL fragment once the changes in <https://github.com/rust-lang/reference/pull/1512> reach stable.

object safe</a>.</div>",
<code>dyn Trait</code> types</a>.</div>",
Copy link
Member

@fmease fmease Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I guess "dyn Trait types" is fine and easy to understand. The official term is "trait object types" but that might be too jargon-y, it's hard to tell for me ^^' I use the latter all the time.

base = crate::clean::utils::DOC_RUST_LANG_ORG_CHANNEL
),
);
Expand Down
4 changes: 2 additions & 2 deletions tests/rustdoc/trait-object-safe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
#![crate_name = "foo"]

// @has 'foo/trait.Unsafe.html'
// @has - '//*[@class="object-safety-info"]' 'This trait is not object safe.'
// @has - '//*[@class="object-safety-info"]' 'This trait is not compatible with dyn Trait types.'
// @has - '//*[@id="object-safety"]' 'Object Safety'
pub trait Unsafe {
fn foo() -> Self;
}

// @has 'foo/trait.Unsafe2.html'
// @has - '//*[@class="object-safety-info"]' 'This trait is not object safe.'
// @has - '//*[@class="object-safety-info"]' 'This trait is not compatible with dyn Trait types.'
// @has - '//*[@id="object-safety"]' 'Object Safety'
pub trait Unsafe2<T> {
fn foo(i: T);
Expand Down
Loading