It is a common mistake to place the snippets in the wrong folder, where they could be missed. For example #30719#pullrequestreview-2262535007 or commit 84900ac34f6888b7a851d0a6a5885192155f865c.
Fix all issues by adding a simple lint check.
Can be tested by reverting a prior commit that violated the rule and then running the new check:
0git revert 35ef34eab7b36e3c53ed438d74a9b783cbcaec27
1( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo run -- --lint=doc_release_note_snippets )
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
46@@ -45,6 +47,11 @@ fn get_linter_list() -> Vec<&'static Linter> {
47 name: "std_filesystem",
48 lint_fn: lint_std_filesystem
49 },
50+ &Linter {
51+ description: "Check that release note snippets are in the right folder",
Checked the code with multiple invalid files, they were all detected correctly:
0doc/release-notes/release-notes-27064.md
1doc/release-notes/release-notes-27064_b.md
2^^^
3Release note snippets must be put into the doc/ folder directly.
4@@ -5,9 +5,11 @@
5 use std::env;
6 use std::fs;
7 use std::io::ErrorKind;
8-use std::path::{Path, PathBuf};
9+use std::path::PathBuf;
10 use std::process::{Command, ExitCode, Stdio};
11
12+/// A possible error returned by any of the linters. The error string should explain the failure
13+/// type and list all violations.
a different linebreak would make this slightly more readable:
0/// A possible error returned by any of the linters.
1/// The error string should explain the failure type and list all violations.
271+The doc/release-notes/ folder is for archived release notes of previous releases only.
272+ "#,
273+ files.join("\n")
274+ ))
275+ }
276+}
I think we can make this more idiomatic Rust by:
Result;git is common here: 0fn lint_doc_release_note_snippets() -> LintResult {
1 let notes = check_output(git().args(["ls-files", "--", "doc/release-notes"]))?;
2 let snippet = Regex::new(r"release-notes-[^.]+\.md$").unwrap();
3 let snippet_notes = notes.lines().filter(|f| snippet.is_match(f)).collect::<Vec<_>>();
4 match snippet_notes.as_slice() {
5 [] => Ok(()),
6 _ => Err(format!(r#"
7{}
8^^^
9Release note snippets must be put into the doc/ folder directly.
10
11The doc/release-notes/ folder is for archived release notes of previous releases only.
12 "#, snippet_notes.join("\n")))
13 }
14}
Note that this requires adding:
0[dependencies]
1regex = "1.10.6"
to the Cargo.toml file.
Also note that the snippet regex could be pulled out of the method.
using regex to match the name instead of string manipulations, repeating parts of the path and the file’s structure (extension + dot);
Seems overkill to import a dependency to check whether a dot is present in a string view or not.
match a slice of the files to return a
Result; extract intermediary results for clarity;
Sure, I’ll try that tomorrow.
we could like use fs::read_dir(“doc/release-notes”) to iterate the folder, but I see that delegating to git is common here:
git ls-files is required, because we only want to list files in git, not arbitrary untracked files.
Seems overkill to import a dependency to check whether a dot is present in a string view or not
Maybe, in that case we can use shell wildcard patterns with ls-files:
0fn lint_doc_release_note_snippets() -> LintResult {
1 let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
2 let snippet_notes = result.lines().collect::<Vec<_>>();
3 match snippet_notes.as_slice() {
4 [] => Ok(()),
5 _ => Err(format!(r#"
6{}
7^^^
8Release note snippets must be put into the doc/ folder directly.
9
10The doc/release-notes/ folder is for archived release notes of previous releases only.
11 "#, snippet_notes.join("\n")))
12 }
13}
Maybe, in that case we can use shell wildcard patterns with
ls-files:
I think you meant “git pathspec” instead of “shell wildcard patterns”, but I like the suggestion, because it let’s git do everything without requiring any additional code or external dependency.
think you meant “git pathspec” instead of “shell wildcard patterns”
I did indeed confuse the two in this case, thanks for clarifying.
Re-ACK fa3a7ebe5b5db63e4cb4fb6974c028cc7af0b898
Just updated docstring.
ACK fa3a7ebe5b5db63e4cb4fb6974c028cc7af0b898
Verified with two files, I like this new version more.