Repository navigation
Conversation
rustbot
added
the
S-waiting-on-review
| @@ -0,0 +1,24 @@ | |||
| //@ rustfmt-config: issue-7177.toml | |||
There was a problem hiding this comment.
Why do we need the issue-7177.toml if it's just setting error_on_unformatted=false? Can't you do that with the inline config directives?
There was a problem hiding this comment.
We don't need error_on_unformatted=false since that's the default value anyway.
There was a problem hiding this comment.
I'll remove issue-7177.toml and unnecessary inline directives.
I initially added the config to prevent the test from failing with error[internal]: line formatted, but exceeded maximum width when I had both source and target. Now I see there was no need for that.
| @@ -0,0 +1,24 @@ | |||
| //@ rustfmt-config: issue-7177.toml | |||
| //@ rustfmt-wrap_comments: true | |||
| //@ rustfmt-unstable_features: true | |||
There was a problem hiding this comment.
It shouldn't be necessary to set unstable_features in tests.
There was a problem hiding this comment.
Removed rustfmt-unstable_features: true directive.
There was a problem hiding this comment.
If there's no difference between the source and the target then we only need the target file for the idempotence test.
| /// Returns `true` if the given string includes link destination. | ||
| /// The pattern is (e.g. `](<disambiguator>@...)`, `](some/path/...)`) | ||
| fn is_doc_link(s: &str) -> bool { | ||
| let mut rest = s; | ||
|
|
||
| while let Some(start) = rest.find("](") { | ||
| let after = &rest[start + 2..]; | ||
|
|
||
| if let Some(end) = after.find(')') { | ||
| let dest = &after[..end]; | ||
|
|
||
| if dest.contains('@') || dest.contains('/') { | ||
| return true; | ||
| } | ||
| rest = &after[end + 1..]; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
| false | ||
| } |
There was a problem hiding this comment.
As mentioned in #7177 (comment), we probably want to be extra conservative and not break for any inline link
There was a problem hiding this comment.
Thanks for the feedback. I'll update is_doc_link to be more conservative: instead of checking for @ and / inside ](...), it will skip wrapping for any line containing ](...).
Is it okay to also skip wrapping for cases that are not links according to the CommonMark spec, e.g. [a](<b)c, [a](<b)c>, [a](<b>c)? These would be false positives for ](...) detection, but skipping them seems safe and consistent with the conservative approach.
ytmimi
added
S-waiting-on-author
| // 4) No URLs were found in the comment | ||
| // 5) No doc links were found (e.g. `](fn@...)`, `](some/path/...)`) | ||
| // 6) The comment is not a table item |
There was a problem hiding this comment.
We should also update Configurations.md if we change the behavior here
There was a problem hiding this comment.
Updated Configurations.md to mention that wrapping is also skipped for lines containing intra-doc links.
| @@ -0,0 +1,24 @@ | |||
| //@ rustfmt-config: issue-7177.toml | |||
There was a problem hiding this comment.
We don't need error_on_unformatted=false since that's the default value anyway.
This branch has not been deployed
4 participants
Description
Fix splitting lines containing intra-doc link destinations.
Fixes #7177
Problem
Rustfmt splits lines inside intra-doc link destinations that contain
@(e.g.fn@super::foo) or/(e.g.some/path/foo.html), producing broken intra-doc links.Solution
Added a call to a new function in
should_wrap_commentthat returnstrueif the line contains a link destination.Use of LLM
I used LLM only for asking questions about the codebase and wrote code by hand. No agents.