fix: add check for image with embedded link - #16773
fix: add check for image with embedded link#16773Francisco-Andre-Martins wants to merge 1 commit into
Conversation
This fix adds an aditional check, by checking if the previous element was an image, and ignoring the lack of punctuation for a subsequent link. An aditional test was added to account for this case. Closes rust-lang#16439
|
Some changes occurred in clippy_lints/src/doc cc @notriddle |
|
r? @llogiq rustbot has assigned @llogiq. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
Lintcheck changes for 71e4ac9
This comment will be updated if you push new changes |
| current_paragraph = None; | ||
| }, | ||
| Event::InlineHtml(_) | Event::Start(Tag::Image { .. }) | Event::End(TagEnd::Image) => { | ||
| Event::InlineHtml(_) | Event::Start(Tag::Image { .. }) => { |
There was a problem hiding this comment.
It should be any kind of hyperlink, not just an image.
There was a problem hiding this comment.
/// See: <https://example.com>
There was a problem hiding this comment.
However you have the following example:
I like potatoes
Which I think should have a comma at the end.
I like potatoes [potatoes](https://en.wikipedia.org/wiki/Potato)
There was a problem hiding this comment.
@BenjaminBrienen sorry to bother, just to check if this didn't get lost in your inbox
There was a problem hiding this comment.
I agree. Maybe we could base it on whether the URL is bare or not? Or maybe use the : as punctuation if there is only a url after? I would rather not have false positives.
another example that should not trigger the lint:
See: [GitHub: rust-lang/rust-clippy](<https://github.com/rust-lang/rust-clippy>)There was a problem hiding this comment.
of course my remark is not blocking and this is objectively already an improvement, so maybe my comment could be a follow up
There was a problem hiding this comment.
@BenjaminBrienen perhaps using https://docs.rs/pulldown-cmark/latest/pulldown_cmark/enum.LinkType.html#variant.Autolink we can better distinguish link types
There was a problem hiding this comment.
Still taking a look on this, if relevant still
There was a problem hiding this comment.
@BenjaminBrienen I looked into the code, and there is already a test that explicitly considers that autolinks should receive coverage. (I found out when I finally managed to get a fix for it, and found out I was failing tests, lol) . And I am not sure I do want to change the meaning of the lint (unless everyone agrees to it)
enum Exceptions {
/// Question marks are fine?
QuestionMark,
/// Exclamation marks are fine!
ExclamationMark,
/// Ellipses are ok too…
Ellipsis,
/// HTML content is however not checked:
/// <em>Raw HTML is allowed as well</em>
RawHtml,
/// The raw HTML exception actually does the right thing to autolinks:
/// <https://spec.commonmark.org/0.31.2/#autolinks>
//~^ doc_paragraphs_missing_punctuation
MarkdownAutolink,
/// This table introduction ends with a colon:
///
/// | Exception | Note |
/// | -------------- | ----- |
/// | Markdown table | A-ok |
MarkdownTable,
}
|
@llogiq sorry to bother, but it seems that this may have gotten lost in your inbox, if you're not the right maintainer, do you mind pointing me in the right direction? |
|
@llogiq Has this gotten lost? |
|
r? clippy |
|
Hello @Francisco-Andre-Martins, I hope that you don't mind we using this contribution to teach @medzernik some things about reviewing (they were very interested!). cc @medzernik Check out this comment first. This one is a little bit different due to this pull request being about documentation. We parse it will |
|
r? @medzernik |
No problem, glad to help! |
There was a problem hiding this comment.
LGTM, thanks for addressing this issue! ❤️ ฅ•ω•ฅ
Now, let's see how this works.
@rustbot delegate @medzernik
|
✌️ @Francisco-Andre-Martins, you can now merge this pull request! If @blyxyas told you to merge after making some further change, then please make that change and post |
|
Error in my command! I forgot the = @rustbot delegate=@medzernik |
|
Error: Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
Sorry for the ping @Francisco-Andre-Martins, I'll contact the infra team for that last message and see we can fix this issue. |
Seems that @medzernik got delegated the PR, even if triagebot didn't respond. So as soon as they take a second (probably the tenth) look, they're allowed to merge! |
View all comments
This fix adds an aditional check, by checking if the previous element was an image, and ignoring the lack of punctuation for a subsequent link. An aditional test was added to account for this case.
Closes #16439
changelog: [
doc_paragraphs_missing_punctuation]: fixed false positive on images with embedded link and added a test for such a case