-
Notifications
You must be signed in to change notification settings - Fork 1.7k
doc_suspicious_footnotes: lint text that looks like a footnote #14708
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
Conversation
c56ef28
to
1356e76
Compare
Looks good to me, thanks! r? samueltardieu |
samueltardieu is on vacation. Please choose another assignee. |
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.
Great initial patch (and a really good lint idea 👍)! I left some suggestions and some questions.
@blyxyas Okay, I've implemented variants on all your suggestions. |
b0e3325
to
99b16ae
Compare
clippy_lints/src/doc/mod.rs
Outdated
@@ -1147,7 +1186,8 @@ fn check_doc<'a, Events: Iterator<Item = (pulldown_cmark::Event<'a>, Range<usize | |||
// Don't check the text associated with external URLs | |||
continue; | |||
} | |||
text_to_check.push((text, range, code_level)); | |||
text_to_check.push((text, range.clone(), code_level)); | |||
doc_suspicious_footnotes::check(cx, doc, range, &fragments); |
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.
I'm sure that you've already tried this, but is there a reason that you didn't use FootnoteReference
from ķEvent
? You would not have to do span magic and manually parse footnotes and it seems that this function doesn't actually use text
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.
The FootnoteReference
event is only emitted if the code is actually a footnote. This lint fires for text that looks like a footnote reference but isn't.
00f6841
to
40040e1
Compare
I've redone the suggestions using attr metadata instead of string manipulation. This also has test cases for cfg_attr and block doc comments. |
6c70997
to
98426f5
Compare
02eda3b
to
a5f4d5d
Compare
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.
Seems pretty good overall, I'll go open the FCP. Although some refactoring is needed.
&& let Some(this_fragment) = fragments | ||
.fragments | ||
.iter() | ||
.find(|frag| { | ||
let found = this_fragment_start < frag.doc.as_str().len(); | ||
if !found { | ||
this_fragment_start -= frag.doc.as_str().len(); | ||
} | ||
found | ||
}) |
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.
Could you refactor this piece of code so that it's clearer what its doing?
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.
Okay, I've added some comments and simplified it down to just be a for
loop.
.find(|frag| { | ||
let found = this_fragment_start < frag.doc.as_str().len(); | ||
if !found { | ||
this_fragment_start -= frag.doc.as_str().len(); |
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.
Also, could you add a test for this specific line of code? Seems that no tests are triggering this case.
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.
Okay, I've added a test called MultiFragmentFootnote
that checks the behavior of that code.
@rustbot ready |
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.
The last nit (not a clear documentation) and I think this is to be merged! (+ We have team members backing this up)
After the change is done, could you squash these commits? |
8ba6d9f
to
fcfbcc8
Compare
@blyxyas Okay, it's done. |
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.
LGTM, thanks! ❤️
changelog: [
doc_suspicious_footnotes
]: lint for text that looks like a footnote reference but has no definitionThis is an alternative to rust-lang/rust#137803, meant to address the concerns about false positives.
This lint only fires when the apparent footnote reference has a name that's made from pure ASCII digits. This choice is justified by running lintcheck on the top 200 crates, plus the clippy default set:
cargo
uses one in its job_queue module, and that's all it found.cc @GuillaumeGomez