Skip to content

Conversation

@cameron1024
Copy link
Contributor

@cameron1024 cameron1024 commented Feb 1, 2022

I'm not sure about wording, it seems OK to me but happy to change if other people have better ideas

closes #8383


changelog: add [doc_link_with_quotes] lint

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 1, 2022
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

I think we should look at more cases to ensure the false positive rate is low. The best way would presumably to check if the link text contains a valid path, but that may be too complex.

bar()
}

pub fn bar() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a test case against false positives, e.g.

/// ['This week in Rust'](https://this-week-in-rust.org)
fn twir() {}

@cameron1024
Copy link
Contributor Author

The particular case that motivated my example was looking for broken links, but I came across something with a broken link and quotes instead of backticks. Ideally the lint would catch these, but I guess that comes at the cost of false positives. Anecdotally, the use of ['something'] seems to be mostly limited to being already within backticks, but I haven't gathered any data for that 🤷

Though if by "valid path" you mean a much simpler syntactic check (rather than checking if it points to an actual item), that sounds reasonable. i.e.:

  • ['invalid_identifier,.<>/?'] won't trigger the lint
  • ['valid::but::doesnt::exist'] still would

Does that seem like a good way to go?

@llogiq
Copy link
Contributor

llogiq commented Feb 5, 2022

That's exactly what I meant.

@bors
Copy link
Contributor

bors commented Feb 11, 2022

☔ The latest upstream changes (presumably #8411) made this pull request unmergeable. Please resolve the merge conflicts.

@llogiq
Copy link
Contributor

llogiq commented Feb 11, 2022

You will need to rebase on master.

@llogiq
Copy link
Contributor

llogiq commented May 28, 2022

So this has waited long enough, I fixed the conflicts. Thanks for your patience.

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2022

📌 Commit c9be57d has been approved by llogiq

@bors
Copy link
Contributor

bors commented May 28, 2022

⌛ Testing commit c9be57d with merge 39231b4...

@bors
Copy link
Contributor

bors commented May 28, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 39231b4 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

detect ['foo'] (quotes, not backticks) in doc comments

4 participants