Split `doc.rs` up into a subdirectory #11801

y21 · 2023-11-13T18:57:58Z

So, first, sorry for the bad diff. 😅

In #11798, @flip1995 suggested splitting doc.rs up, much like how we have the methods/, matches/, types/ subdirectories.
I agree with this, the file is getting bigger as we add more and more doc lints that it makes sense to do this refactoring.

This is purely an internal change that moves things around a bit.
(EDIT: depending on the outcome of #11801 (comment) , this may change the lint group name from doc_markdoc to doc).

I tried to not change any of the actual logic of the lints and as such some things weren't as easy to move to a separate file. So we still have some span_lint* calls in the doc/mod.rs file, which I think is fine. This is also the case in methods/mod.rs.

Also worth mentioning that the lints missing_errors_doc, missing_panics_doc, missing_safety_doc and unnecessary_safety_doc have a lot of the same logic so it didn't make much sense for each of these to be in their own file. Instead I just put them all in missing_headers.rs

I also added a bit of documentation to the involved check_{attrs,doc} methods.

changelog: none

rustbot · 2023-11-13T18:58:03Z

r? @blyxyas

(rustbot has picked a reviewer for you, use r? to override)

y21 · 2023-11-13T19:00:36Z

clippy_lints/src/doc/suspicious_doc_comments.rs

+    }
+}
+
+fn collect_doc_replacements(attrs: &[Attribute]) -> Vec<(Span, String)> {


The code in here was previously in the same function where we emit the lint but I extracted that into a separate function because I thought that looks better. If you disagree, I can change it back to have it all in the same function.

I don't disagree, but if we're not just moving some functions into other files, you could also refactor this commit.

With this lint specifically I'm more familiar since I wrote it myself, and, actually that's how it looked like before my PR #11798 where I put them into one to save space since the file was getting large.
Do you have anything in mind that can be refactored in that other commit?

@blyxyas ? :)
(the file is large, wondering if you want something specific refactored)

blyxyas · 2023-11-15T19:02:26Z

Could you please split this into a lint per commit 🙏 so that I can review better and the commit history is clearer (they won't be squashed on merge) (^..^)ﾉ

bors · 2023-11-16T19:06:52Z

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

blyxyas

Is there any other code change other than just changing code to files? If there isn't (outside that comment), I think this is pretty much ready.

clippy_lints/src/doc/link_with_quotes.rs

blyxyas · 2023-11-17T13:01:57Z

clippy_lints/src/doc/suspicious_doc_comments.rs

+    }
+}
+
+fn collect_doc_replacements(attrs: &[Attribute]) -> Vec<(Span, String)> {


I don't disagree, but if we're not just moving some functions into other files, you could also refactor this commit.

y21 · 2023-11-17T13:10:28Z

Is there any other code change other than just changing code to files?

Only in suspicious_doc_comments. I also added a FindPanicUnwrap::find_span function that can be used in a bunch of places in the main mod.rs.
Other lints are unchanged.

y21 · 2023-11-17T16:08:37Z

Oh, the lint group was also changed from DocMarkdown to Doc. I initially thought that could be a small internal change that the user wouldn't notice, but #![warn(clippy::doc_markdown)] works, and won't anymore after this. Should I just change it back? I think doc is a better name for the group since it's really not about markdown anymore at that point, but I'm not sure if that's reason enough to do this change.

blyxyas · 2023-11-17T16:20:45Z

I'll nominate this issue, let's see what the team thinks about this. If we were to change its name, it would need to be deprecated some time before being removed =＾● ⋏ ●＾=

y21 · 2023-11-17T16:28:32Z

If this ends up blocking the PR, I guess I could also just revert it for this PR, just so that this can remain an "internal-only" PR.
Renaming from DocMarkdown to Doc, if the team agrees, could be its own PR since that's a rather self-contained thing not relevant to this splitup.

Since this touches quite a lot of code and is prone to bit rot whenever someone makes a change to doc lints (and solving conflicts here is kind of a chore), I wouldn't want to leave this open for too long

blyxyas · 2023-11-19T21:32:20Z

Okis, rename the pass to DocMarkdown for now, we'll still discuss it with the rest of the team, but let's keep the ball rolling (=^･ω･^)y＝

I think that the review should be addressed, and that this would be pretty ready to be merge-ready!

blyxyas

LGTM, thanks! ❤️ I've revised these like 5 times and I still feel like there's something obvious that I don't see. Quite the GuillaumeGomez syndrome (=^-ω-^=)

GuillaumeGomez · 2023-11-20T12:33:54Z

😆 Careful, it doesn't get better with time. ^^'

blyxyas · 2023-11-20T13:25:17Z

Oh no, I just remember that I said that they wouldn't be squashed ;(
Sorry for the chore, Timo, it's just my Alzheimer. I feel guilty now D:

blyxyas · 2023-11-20T13:26:08Z

@bors r+

bors · 2023-11-20T13:27:50Z

📌 Commit 56cee3c has been approved by blyxyas

It is now in the queue for this repository.

bors · 2023-11-20T13:30:05Z

⌛ Testing commit 56cee3c with merge 0487ed5...

Split `doc.rs` up into a subdirectory So, first, sorry for the bad diff. 😅 In #11798, `@flip1995` suggested splitting `doc.rs` up, much like how we have the `methods/`, `matches/`, `types/` subdirectories. I agree with this, the file is getting bigger as we add more and more doc lints that it makes sense to do this refactoring. This is purely an internal change that moves things around a bit. (**EDIT:** depending on the outcome of #11801 (comment) , this may change the lint group name from `doc_markdoc` to `doc`). I tried to not change any of the actual logic of the lints and as such some things weren't as easy to move to a separate file. So we still have some `span_lint*` calls in the `doc/mod.rs` file, which I think is fine. This is also the case in `methods/mod.rs`. Also worth mentioning that the lints missing_errors_doc, missing_panics_doc, missing_safety_doc and unnecessary_safety_doc have a lot of the same logic so it didn't make much sense for each of these to be in their own file. Instead I just put them all in `missing_headers.rs` I also added a bit of documentation to the involved `check_{attrs,doc}` methods. changelog: none

y21 · 2023-11-20T14:49:32Z

Did bors get stuck? 🤔
It's been going for a while

flip1995 · 2023-11-20T17:16:23Z

It lost track of this PR somehow. I re-synchronized bors for Clippy. Let's see if that helps.

flip1995 · 2023-11-20T17:18:56Z

Now it at least shows up as approved in the queue. Let's

@bors retry

so that it will be merged.

bors · 2023-11-20T17:19:53Z

⌛ Testing commit 56cee3c with merge 157ab49...

Split `doc.rs` up into a subdirectory So, first, sorry for the bad diff. 😅 In #11798, `@flip1995` suggested splitting `doc.rs` up, much like how we have the `methods/`, `matches/`, `types/` subdirectories. I agree with this, the file is getting bigger as we add more and more doc lints that it makes sense to do this refactoring. This is purely an internal change that moves things around a bit. (**EDIT:** depending on the outcome of #11801 (comment) , this may change the lint group name from `doc_markdoc` to `doc`). I tried to not change any of the actual logic of the lints and as such some things weren't as easy to move to a separate file. So we still have some `span_lint*` calls in the `doc/mod.rs` file, which I think is fine. This is also the case in `methods/mod.rs`. Also worth mentioning that the lints missing_errors_doc, missing_panics_doc, missing_safety_doc and unnecessary_safety_doc have a lot of the same logic so it didn't make much sense for each of these to be in their own file. Instead I just put them all in `missing_headers.rs` I also added a bit of documentation to the involved `check_{attrs,doc}` methods. changelog: none

bors · 2023-11-20T17:22:08Z

⌛ Testing commit 56cee3c with merge 72c0d80...

flip1995 · 2023-11-20T17:24:50Z

First bors didn't want to do anything and now they do it twice. Weird fella.

bors · 2023-11-20T17:31:41Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 72c0d80 to master...

…Manishearth rename `DocMarkdown` pass to `Documentation` Followup of #11801 This was discussed in todays meeting: https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/Meeting.202023-11-28/near/404667082 Decided to go with `Documentation`, because this would also make it consistent with how `Attributes` is named in `attrs.rs` (and it also sounds good). changelog: none

rustbot assigned blyxyas Nov 13, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 13, 2023

y21 commented Nov 13, 2023

View reviewed changes

y21 force-pushed the split_doc_pass branch from 924888d to f1b5434 Compare November 15, 2023 22:14

y21 force-pushed the split_doc_pass branch from f1b5434 to 1daf572 Compare November 16, 2023 19:44

blyxyas reviewed Nov 17, 2023

View reviewed changes

blyxyas added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Nov 17, 2023

blyxyas mentioned this pull request Nov 19, 2023

Add new check_private_items config #11842

Merged

y21 force-pushed the split_doc_pass branch from c635ffe to d6abdf5 Compare November 19, 2023 22:03

blyxyas approved these changes Nov 20, 2023

View reviewed changes

move doc.rs to its own subdirectory

56cee3c

y21 force-pushed the split_doc_pass branch from d6abdf5 to 56cee3c Compare November 20, 2023 11:15

bors merged commit 72c0d80 into rust-lang:master Nov 20, 2023

y21 mentioned this pull request Nov 28, 2023

rename DocMarkdown pass to Documentation #11892

Merged

flip1995 removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `doc.rs` up into a subdirectory #11801

Split `doc.rs` up into a subdirectory #11801

y21 commented Nov 13, 2023 •

edited

Loading

rustbot commented Nov 13, 2023

y21 Nov 13, 2023

blyxyas Nov 17, 2023

y21 Nov 17, 2023

y21 Nov 19, 2023 •

edited

Loading

blyxyas commented Nov 15, 2023

bors commented Nov 16, 2023

blyxyas left a comment

blyxyas Nov 17, 2023

y21 commented Nov 17, 2023

y21 commented Nov 17, 2023

blyxyas commented Nov 17, 2023

y21 commented Nov 17, 2023 •

edited

Loading

blyxyas commented Nov 19, 2023 •

edited

Loading

blyxyas left a comment •

edited

Loading

GuillaumeGomez commented Nov 20, 2023

blyxyas commented Nov 20, 2023

blyxyas commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

y21 commented Nov 20, 2023

flip1995 commented Nov 20, 2023

flip1995 commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

flip1995 commented Nov 20, 2023

bors commented Nov 20, 2023

Split doc.rs up into a subdirectory #11801

Split doc.rs up into a subdirectory #11801

Conversation

y21 commented Nov 13, 2023 • edited Loading

rustbot commented Nov 13, 2023

y21 Nov 13, 2023

Choose a reason for hiding this comment

blyxyas Nov 17, 2023

Choose a reason for hiding this comment

y21 Nov 17, 2023

Choose a reason for hiding this comment

y21 Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

blyxyas commented Nov 15, 2023

bors commented Nov 16, 2023

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas Nov 17, 2023

Choose a reason for hiding this comment

y21 commented Nov 17, 2023

y21 commented Nov 17, 2023

blyxyas commented Nov 17, 2023

y21 commented Nov 17, 2023 • edited Loading

blyxyas commented Nov 19, 2023 • edited Loading

blyxyas left a comment • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez commented Nov 20, 2023

blyxyas commented Nov 20, 2023

blyxyas commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

y21 commented Nov 20, 2023

flip1995 commented Nov 20, 2023

flip1995 commented Nov 20, 2023

bors commented Nov 20, 2023

bors commented Nov 20, 2023

flip1995 commented Nov 20, 2023

bors commented Nov 20, 2023

Split `doc.rs` up into a subdirectory #11801

Split `doc.rs` up into a subdirectory #11801

y21 commented Nov 13, 2023 •

edited

Loading

y21 Nov 19, 2023 •

edited

Loading

y21 commented Nov 17, 2023 •

edited

Loading

blyxyas commented Nov 19, 2023 •

edited

Loading

blyxyas left a comment •

edited

Loading