-
Notifications
You must be signed in to change notification settings - Fork 38.7k
test: [move-only] Move lint functions into modules #34098
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
base: master
Are you sure you want to change the base?
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34098. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsNo conflicts as of last run. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-01-05 |
|
IIRC @davidgumberg had a similar patch a year or two ago, but moved each function into a separate file. This patch keeps a grouping into topics. Maybe @davidgumberg can nack or ack this one? |
|
Concept ACK fafdeb8 on an initial glance, will take a deeper look. The context based code separation seems fine to me, also makes the Rust code in the linter slightly more inviting to me, who has limited experience with it. |
Also, rename lint_doc to lint_doc_args.
Also, run cargo fmt on main.rs
fafdeb8 to
fa578d9
Compare
|
Inlined the changes back to see the diff, besides some whitespace, it basically only contained exposing the functions as public and renaming Detailsdiff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index 171c98072a..acd36ac2dd 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -2,18 +2,27 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or https://opensource.org/license/mit/.
+mod lint_cpp;
+mod lint_docs;
+mod lint_py;
+mod lint_repo_hygiene;
+mod lint_text_format;
+mod util;
+
use std::env;
-use std::fs::{self, File};
-use std::io::{ErrorKind, Read, Seek, SeekFrom};
-use std::path::PathBuf;
-use std::process::{Command, ExitCode, Stdio};
+use std::fs;
+use std::process::{Command, ExitCode};
-/// A possible error returned by any of the linters.
-///
-/// The error string should explain the failure type and list all violations.
-type LintError = String;
-type LintResult = Result<(), LintError>;
-type LintFn = fn() -> LintResult;
+use lint_cpp::{
+ lint_boost_assert, lint_includes_build_config, lint_rpc_assert, lint_std_filesystem,
+};
+use lint_docs::{lint_doc_args, lint_doc_release_note_snippets, lint_markdown};
+use lint_py::lint_py_lint;
+use lint_repo_hygiene::{lint_scripted_diff, lint_subtree};
+use lint_text_format::{
+ lint_commit_msg, lint_tabs_whitespace, lint_trailing_newline, lint_trailing_whitespace,
+};
+use util::{check_output, commit_range, get_git_root, git, LintFn, LintResult};
struct Linter {
pub description: &'static str,
@@ -26,7 +35,7 @@ fn get_linter_list() -> Vec<&'static Linter> {
&Linter {
description: "Check that all command line arguments are documented.",
name: "doc",
- lint_fn: lint_doc
+ lint_fn: lint_doc_args
},
&Linter {
description: "Check that no symbol from bitcoin-build-config.h is used without the header being included",
@@ -162,7 +171,7 @@ fn parse_lint_args(args: &[String]) -> Vec<&'static Linter> {
/// Lint functions should use this command, so that only files tracked by git are considered and
/// temporary and untracked files are ignored. For example, instead of 'grep', 'git grep' should be
/// used.
-fn git() -> Command {
+pub fn git() -> Command {
let mut git = Command::new("git");
git.arg("--no-pager");
git
@@ -170,7 +179,7 @@ fn git() -> Command {
/// Return stdout on success and a LintError on failure, when invalid UTF8 was detected or the
/// command did not succeed.
-fn check_output(cmd: &mut std::process::Command) -> Result<String, LintError> {
+pub fn check_output(cmd: &mut Command) -> Result<String, LintError> {
let out = cmd.output().expect("command error");
if !out.status.success() {
return Err(String::from_utf8_lossy(&out.stderr).to_string());
@@ -184,12 +193,12 @@ fn check_output(cmd: &mut std::process::Command) -> Result<String, LintError> {
}
/// Return the git root as utf8, or panic
-fn get_git_root() -> PathBuf {
+pub fn get_git_root() -> PathBuf {
PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap())
}
/// Return the commit range, or panic
-fn commit_range() -> String {
+pub fn commit_range() -> String {
// Use the env var, if set. E.g. COMMIT_RANGE='HEAD~n..HEAD' for the last 'n' commits.
env::var("COMMIT_RANGE").unwrap_or_else(|_| {
// Otherwise, assume that a merge commit exists. This merge commit is assumed
@@ -204,7 +213,7 @@ fn commit_range() -> String {
}
/// Return all subtree paths
-fn get_subtrees() -> Vec<&'static str> {
+pub fn get_subtrees() -> Vec<&'static str> {
// Keep in sync with [test/lint/README.md#git-subtree-checksh]
vec![
"src/crc32c",
@@ -217,7 +226,7 @@ fn get_subtrees() -> Vec<&'static str> {
}
/// Return the pathspecs to exclude by default
-fn get_pathspecs_default_excludes() -> Vec<String> {
+pub fn get_pathspecs_default_excludes() -> Vec<String> {
get_subtrees()
.iter()
.chain(&[
@@ -227,7 +236,7 @@ fn get_pathspecs_default_excludes() -> Vec<String> {
.collect()
}
-fn lint_subtree() -> LintResult {
+pub fn lint_subtree() -> LintResult {
// This only checks that the trees are pure subtrees, it is not doing a full
// check with -r to not have to fetch all the remotes.
let mut good = true;
@@ -245,7 +254,7 @@ fn lint_subtree() -> LintResult {
}
}
-fn lint_scripted_diff() -> LintResult {
+pub fn lint_scripted_diff() -> LintResult {
if Command::new("test/lint/commit-script-check.sh")
.arg(commit_range())
.status()
@@ -258,7 +267,7 @@ fn lint_scripted_diff() -> LintResult {
}
}
-fn lint_commit_msg() -> LintResult {
+pub fn lint_commit_msg() -> LintResult {
let mut good = true;
let commit_hashes = check_output(git().args([
"-c",
@@ -293,7 +302,7 @@ fn lint_commit_msg() -> LintResult {
}
}
-fn lint_py_lint() -> LintResult {
+pub fn lint_py_lint() -> LintResult {
let bin_name = "ruff";
let checks = format!(
"--select={}",
@@ -353,16 +362,14 @@ fn lint_py_lint() -> LintResult {
Ok(status) if status.success() => Ok(()),
Ok(_) => Err(format!("`{bin_name}` found errors!")),
Err(e) if e.kind() == ErrorKind::NotFound => {
- println!(
- "`{bin_name}` was not found in $PATH, skipping those checks."
- );
+ println!("`{bin_name}` was not found in $PATH, skipping those checks.");
Ok(())
}
Err(e) => Err(format!("Error running `{bin_name}`: {e}")),
}
}
-fn lint_std_filesystem() -> LintResult {
+pub fn lint_std_filesystem() -> LintResult {
let found = git()
.args([
"grep",
@@ -390,7 +397,7 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
}
}
-fn lint_rpc_assert() -> LintResult {
+pub fn lint_rpc_assert() -> LintResult {
let found = git()
.args([
"grep",
@@ -420,7 +427,7 @@ checks should be used over assert. See: src/util/check.h
}
}
-fn lint_boost_assert() -> LintResult {
+pub fn lint_boost_assert() -> LintResult {
let found = git()
.args([
"grep",
@@ -446,7 +453,7 @@ include of the boost/assert.hpp dependency.
}
}
-fn lint_doc_release_note_snippets() -> LintResult {
+pub fn lint_doc_release_note_snippets() -> LintResult {
let non_release_notes = check_output(git().args([
"ls-files",
"--",
@@ -497,7 +504,7 @@ fn get_pathspecs_exclude_whitespace() -> Vec<String> {
list
}
-fn lint_trailing_whitespace() -> LintResult {
+pub fn lint_trailing_whitespace() -> LintResult {
let trailing_space = git()
.args(["grep", "-I", "--line-number", "\\s$", "--"])
.args(get_pathspecs_exclude_whitespace())
@@ -522,7 +529,7 @@ sourced files to the exclude list.
}
}
-fn lint_trailing_newline() -> LintResult {
+pub fn lint_trailing_newline() -> LintResult {
let files = check_output(
git()
.args([
@@ -560,7 +567,7 @@ Please add any false positives to the exclude list.
}
}
-fn lint_tabs_whitespace() -> LintResult {
+pub fn lint_tabs_whitespace() -> LintResult {
let tabs = git()
.args(["grep", "-I", "--line-number", "--perl-regexp", "^\\t", "--"])
.args(["*.cpp", "*.h", "*.md", "*.py", "*.sh"])
@@ -584,7 +591,7 @@ Please add any false positives, such as subtrees, or externally sourced files to
}
}
-fn lint_includes_build_config() -> LintResult {
+pub fn lint_includes_build_config() -> LintResult {
let config_path = "./cmake/bitcoin-build-config.h.in";
let defines_regex = format!(
r"^\s*(?!//).*({})",
@@ -673,7 +680,7 @@ the header. Consider removing the unused include.
Ok(())
}
-fn lint_doc() -> LintResult {
+pub fn lint_doc_args() -> LintResult {
if Command::new("test/lint/check-doc.py")
.status()
.expect("command error")
@@ -685,7 +692,7 @@ fn lint_doc() -> LintResult {
}
}
-fn lint_markdown() -> LintResult {
+pub fn lint_markdown() -> LintResult {
let bin_name = "mlc";
let mut md_ignore_paths = get_subtrees();
md_ignore_paths.push("./doc/README_doxygen.md");
@@ -728,6 +735,7 @@ Markdown link errors found:
}
}
+
fn run_all_python_linters() -> LintResult {
let mut good = true;
let lint_dir = get_git_root().join("test/lint");Lightly tested code review ACK fa578d9 |
The single, large
main.rsfile is fine, but at some point it becomes harder to read.So reduce the size by pulling functions out into modules.
This can be reviewed with the git option:
--color-moved=dimmed-zebra