Rewrite the project in Rust #49
Conversation
Temporarily hardcoded some stuff...
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a Rust workspace implementing a new mwutil CLI (many modules, types, and utilities), Rust CI linting and Dependabot updates, a Rust Cargo manifest and .gitignore, renames the Python CLI entry, and removes one supported MediaWiki branch constant. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI (User)
participant Loader as Config Loader
participant Compose as Docker Compose
participant Container as Service Container
participant DB as Database
participant FS as Filesystem
User->>Loader: parse args, load MWUtilConfig
Loader-->>User: return config
User->>Compose: build docker-compose command (env-file, workdir)
Compose->>Container: docker exec ... (run command/script)
Container->>DB: run DB client (dump/query) or invoke maintenance script
DB-->>Container: return output/status
Container->>FS: write/read files (dumps, patches)
Container-->>Compose: command exit status
Compose-->>User: return status and streamed output
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@rs/src/config.rs`:
- Around line 113-114: The error context for reading the COMPOSE_PROFILES env
var has a typo; update the context string passed to env::var on the
compose_profiles line so it references "COMPOSE_PROFILES" (not
"COMPOSER_PROFILES"); locate the env::var call that initializes compose_profiles
and change the .context(...) message accordingly to match the actual environment
variable name.
- Around line 189-193: enable_profile currently appends the given profile to
config.compose_profiles unconditionally, causing duplicate entries; modify
enable_profile (and/or update_profiles if preferred) to first check whether
profile already exists in config.compose_profiles (e.g., via contains) and only
push and call update_profiles when it's not present (or alternatively append
then deduplicate the vector before calling update_profiles) so duplicates are
not added; reference symbols: enable_profile, config.compose_profiles,
update_profiles.
- Around line 172-182: The update_env_var function currently only performs a
regex replace and silently does nothing if the variable is missing; update it so
after building the Regex (in update_env_var) you check whether the regex matches
the file (e.g., re.is_match(&contents) or by inspecting the result of
re.replace_all) and, if no match, append a newline plus format!("{}={}", var,
val) to the file contents before writing; preserve the existing write path
(fs::write(env_file, ...).context(...)) and ensure you still use
regex::escape(var) for the pattern to avoid accidental partial matches.
In `@rs/src/main.rs`:
- Around line 87-89: load_mwutil_config errors are being swallowed by calling
config.as_ref().ok() before calling cli.module.run_globally; preserve and report
the error when loading fails so users see parse/load problems for non-global
modules. Change the call site to match on the Result returned by
load_mwutil_config (or use if let Err(e) = ...) and when Err(e) occurs, log or
print the error (e.g., via eprintln! or the existing logger) before calling
cli.module.run_globally with None; when Ok(cfg) pass Some(cfg.as_ref()) so
run_globally still gets the config when available. Ensure you reference
load_mwutil_config, config, and cli.module.run_globally when making the change.
In `@rs/src/modules/clone.rs`:
- Around line 83-85: The Composer module is being instantiated without its
folder context, so it runs in the wrong directory; update the call that creates
Modules::Composer in this file to supply the cloned repository path as the
folder field (e.g., construct Composer with folder set to the repo's path from
the clone result or args) before calling run(config). Locate Modules::Composer
instantiation and replace Default::default() with a Composer value that includes
folder: <cloned_repo_path_variable> (or equivalent field name used in
rs/src/modules/composer.rs) so that Composer::run executes with the correct
current_dir.
In `@rs/src/modules/db.rs`:
- Around line 274-283: The temporary dump isn't removed if import_dump fails;
add a scope guard that calls delete_dump in Drop to ensure cleanup: after
creating the dump and dump_name, instantiate a DumpCleanup (or similar) holding
&new_config and dump_name and a keep flag; implement Drop to call
delete_dump(&config, DumpSubArgs { name: name.clone() }) when keep is false;
after the normal successful delete_dump call set cleanup.keep = true so the
guard doesn't delete twice. Ensure this references the existing functions/types
import_dump, delete_dump, DumpSubArgs and the local dump_name/new_config used in
switch().
- Around line 260-272: The loop that polls the DB readiness using
run_sql_query(&new_config, DbCommandUser::Mw, Some(DbCommandDatabase::Mw),
"SELECT 1;") can spin forever; modify the readiness logic in the function
containing this loop to use a max retry count or a timeout (e.g., record start =
Instant::now() and loop while Instant::now().duration_since(start) < max_wait or
iterate a counter up to max_retries), and when exceeded return an Err or
propagate a clear error instead of looping indefinitely; ensure the code
references run_sql_query and the surrounding function's return type is updated
to return a Result so callers can handle the failure.
In `@rs/src/modules/list_repo_remotes.rs`:
- Around line 37-45: The code unconditionally calls pop() on the String named
remote before inserting it into remotes (the remotes.insert(...) block), which
can truncate data if git output lacks a trailing newline; replace the pop() with
a safe trim of trailing newline/whitespace (e.g., use remote =
remote.trim_end().to_string()) or check that remote.ends_with('\n') before
popping so the actual URL isn't accidentally shortened when building the remotes
map (refer to the remote variable and remotes.insert(...) for where to make the
change).
In `@rs/src/modules/opensearch.rs`:
- Around line 58-62: The call is mutating a temporary because
enable_profile(&mut config.clone(), profile) passes a mutable reference to a
clone, so changes are lost; change the call to pass the real mutable config
(e.g., enable_profile(&mut config, profile)) or adjust enable_profile's
signature to take and return an owned Config if you intend to work on a copy,
and if profile changes must be persisted update the caller to write the modified
config to disk (or return the updated config) so mutations are not silently
dropped; locate enable_profile and the caller in this diff to implement the
correct calling convention.
In `@rs/src/modules/reset.rs`:
- Around line 61-74: The current block silently ignores errors from
fs::read_dir(upload_dir); change it to propagate the error instead of swallowing
it (e.g., replace the if let Ok(files) pattern with using
fs::read_dir(upload_dir)? and iterate over the resulting iterator), so failures
to open the upload directory surface to the caller; ensure the enclosing
function's signature returns a compatible Result so the ? operator compiles and
keep the existing logic that filters by EXCLUDED_UPLOADS and removes
files/directories.
In `@rs/src/modules/security.rs`:
- Around line 49-51: The code uses fs::create_dir(&folder) which will fail if
parent directories are missing; change this to fs::create_dir_all(&folder)? when
creating the security patch folder (the block that checks folder.exists()) so
the directory tree is created recursively (keep the existing error propagation
behavior by returning the Result from the function).
In `@rs/src/modules/setup_repo.rs`:
- Around line 43-47: The error message is misleading: the code calls
set_git_config with config.git_username but the anyhows!("git-review username
not set!") refers to a different name; update the error to match the symbol by
changing the message to something like "git username not set!" (i.e., replace
"git-review username not set!" with a message referencing git_username) or, if a
distinct gitreview username field is intended, read that field (e.g.,
config.gitreview_username) instead of config.git_username and keep the original
message; ensure the fix is applied where set_git_config is invoked and
references config.git_username (or the corrected field) so the config key and
error text are consistent.
- Around line 49-53: The call to Command::new("git").args(["review", "-s",
"--verbose"]).current_dir(repo_folder).status() obtains a process exit status
but doesn't validate it; change the code to capture the returned Status, check
status.success(), and return an Err (propagated or converted to an appropriate
io::Error) when it is false so the function does not return Ok(()) on failure;
update the callers if necessary to propagate the error from this setup function
(referencing the Command::new("git") invocation, the .args([...]) call,
.current_dir(repo_folder), and the .status() result).
In `@rs/src/utils.rs`:
- Around line 36-41: The regex capture currently returns the whole match because
the code maps captures to c.get(0).unwrap().as_str(), which yields the entire
"'MW_VERSION', 'x.y.z'" string; change the mapping to use the first capture
group c.get(1).unwrap().as_str() so MWVersion::parse receives only the version
token, i.e. update the closure that maps captures before calling
MWVersion::parse (references: the Regex variable re, the captures call, and
MWVersion::parse).
- Around line 14-16: The container_completer uses expect on find_base_dir which
will panic and crash shell completion; change container_completer to handle the
failure gracefully by matching the result/option from find_base_dir (e.g., if
let Ok(dir) / Some(dir) else { return Vec::new() }) and return an empty Vec of
CompletionCandidate or a safe fallback instead of panicking, optionally logging
the error via existing logging/tracing rather than calling expect; update
references to base_dir inside container_completer accordingly so no
unwrap/expect remains.
- Around line 44-51: The set_git_config function currently ignores git's exit
status and always returns Ok(()); modify set_git_config to inspect the
Command::status() result (from status() on the Command in set_git_config) and
return an error when the process exit is non-success (e.g., check
status.success()), propagating a contextual error that includes the exit code or
standard error text; ensure you still use anyhow::Context for the spawn error
and then map a non-success exit into an anyhow::Error with a clear message
referencing the git command, option, and repo_folder so failures don't get
treated as successful.
🧹 Nitpick comments (16)
rs/Cargo.toml (1)
15-15: Consider usingdotenvyinstead ofdotenv.The
dotenvcrate is unmaintained;dotenvyis the actively maintained fork with the same API. This avoids potential future security or compatibility issues.Suggested change
-dotenv = "0.15.0" +dotenvy = "0.15"rs/src/modules/composer.rs (1)
28-31: Exit status is silently discarded.The
.ok()call discards theExitStatus, so a failingcomposer update(non-zero exit code) returnsOk(()). This matchesbash.rsbut differs fromsql.rswhich bails on failure. If silent failure is intentional for interactive commands, consider adding a brief comment to clarify this design choice.rs/src/modules/info.rs (1)
37-37: Avoid unnecessary clone beforeunwrap_or.
clone()allocates a newStringeven whenmw_databaseisSome. Useas_deref()to borrow instead.♻️ Suggested fix
- ("MW Database", &config.mw_database.clone().unwrap_or("Unknown".into())), + ("MW Database", config.mw_database.as_deref().unwrap_or("Unknown")),rs/src/modules/pull.rs (2)
6-15: Address the TODO to reduce duplication.The comment acknowledges duplication between
PullRepoTypeandRepoType. SincePullRepoTypeaddsCoreandConfigvariants not present inRepoType, consider extendingRepoTypeto include these or creating a trait to unify the behavior.Would you like me to open an issue to track this refactoring task?
38-41: Consider reporting git pull failures.The exit status from
git pullis discarded. Unlike interactive commands, git pull failures (merge conflicts, network errors, dirty working tree) may warrant user notification.♻️ Optional: Check exit status
- Command::new("git") + let status = Command::new("git") .arg("pull") .current_dir(dir) .status()?; + if !status.success() { + anyhow::bail!("git pull failed with status: {}", status); + } + Ok(())rs/src/modules/lint.rs (1)
56-70: Unnecessary.clone()onfolder.On line 60,
folder.clone()is unnecessary sinceOption<String>can be moved directly here. Thefolderparameter is already owned.♻️ Suggested simplification
fn execute(&self, config: &MWUtilConfig, folder: Option<String>, args: &[&str]) -> anyhow::Result<ExitStatus> { let mut cmd = self.base_command(); cmd.args(args); - if let Some(folder) = folder.clone() { + if let Some(folder) = folder { cmd.current_dir(PathBuf::from_str(folder.as_str())?); }rs/src/modules/list_repo_remotes.rs (1)
30-36: Silent failure on git command errors hides issues.When the git command fails (e.g., corrupt repo, permission issues), the error is silently ignored with
continue. Consider logging a warning so users know which repos couldn't be queried.♻️ Optional: Add warning for failed git commands
let status_res = Command::new("git") .args(["config", "--get", "remote.origin.url"]) .current_dir(&path) .output(); let Ok(status) = status_res else { + eprintln!("Warning: Failed to get remote for {:?}", path); continue; };rs/src/modules/clone.rs (2)
55-69: Magic number32768should be documented or use a constant.The value
32768represents exit status 128 in the raw Unix process status format (128 << 8). Git uses exit code 128 for fatal errors. Consider extracting this as a named constant or adding a comment explaining its meaning for maintainability.♻️ Add clarity to the magic number
+ // Git exit code 128 (fatal error) appears as 32768 in raw status (128 << 8) + const GIT_FATAL_ERROR_RAW: i32 = 32768; println!("Git clone exited with status: {}", status); - if status.into_raw() == 32768 { + if status.into_raw() == GIT_FATAL_ERROR_RAW {
130-135: Same unwrap issue and inefficient regex compilation.Line 130 has the same potential panic as line 106. Additionally,
Regex::new()is called every time this function runs. For a static pattern, consider usingstd::sync::LazyLock(oronce_cell/lazy_static) to compile it once.♻️ Use LazyLock for the regex
+use std::sync::LazyLock; + +static GITHUB_NAME_RE: LazyLock<Regex> = LazyLock::new(|| { + Regex::new(r"mediawiki-(?:extension|skin|service)s?-(.*)").unwrap() +}); + // In get_repo_data: - let mut name = repo.split("/").last().unwrap().to_string(); - let re = Regex::new(r"mediawiki-(?:extension|skin|service)s?-(.*)") - .unwrap(); - if let Some(matched_name) = re.captures(&name).and_then(|c| c.get(1)) { + let mut name = repo.rsplit('/').next().unwrap_or(&repo).to_string(); + if let Some(matched_name) = GITHUB_NAME_RE.captures(&name).and_then(|c| c.get(1)) {rs/src/modules/run.rs (1)
74-81:expect()could panic on non-UTF8 filenames.Line 76 uses
expect("Failed to decode file name string")which will panic if a filename contains invalid UTF-8. While rare, consider usingto_string_lossy()for robustness, consistent with howlist_repo_remotes.rshandles filenames.♻️ Use lossy conversion for consistency
if path.is_file() && path.extension() == Some("php".as_ref()) && let Some(name) = path.file_name() { if let Some(prefix) = prefix { - let name = name.to_str().expect("Failed to decode file name string"); + let name = name.to_string_lossy(); result.push(CompletionCandidate::new(format!("{prefix}:{name}"))); } else { result.push(CompletionCandidate::new(name));rs/src/modules/security.rs (1)
64-66: Consider caching the compiled regex.The regex is recompiled on every invocation. For a CLI tool this is minor, but you could use
std::sync::LazyLock(stable in Rust 1.80+) or theonce_cellcrate to compile it once.Example with LazyLock
use std::sync::LazyLock; static TASK_ID_REGEX: LazyLock<Regex> = LazyLock::new(|| { Regex::new(r"^T[0-9]{4,10}$").unwrap() }); // Then use: if TASK_ID_REGEX.is_match(branch_name.as_str()) {rs/src/modules/bash.rs (1)
34-37: Unnecessary clone beforesplit_first().
split_first()works on slices and doesn't consume the vector. You can avoid cloning by usingas_slice().Proposed fix
- let (program, cmd_args) = match args.command.clone().split_first() { - Some((first, rest)) => (first.clone(), rest.to_vec()), + let (program, cmd_args) = match args.command.split_first() { + Some((first, rest)) => (first.to_owned(), rest.to_vec()), None => ("bash".to_string(), vec![]) };rs/src/main.rs (1)
97-100: The safety ofunwrap()calls depends onworks_globally()being correct.The pattern here is safe as long as
works_globally()accurately identifies all modules that can operate without config. However, if a new module is added that works globally but the developer forgets to updateworks_globally(), this would panic instead of showing a helpful error.Consider using a method that returns
Option<&MWUtilConfig>for global modules and&MWUtilConfigfor others, or restructuring to make the compiler enforce this invariant.rs/src/modules/reset.rs (1)
28-50: Spinner progress count may be incorrect.The spinner
maxis set toactions.len(), butspinner.next()is only called for actions that are present in the vector. If the user specifies[Database, OpenSearch](2 items), the spinner will show 1/2 and 2/2 correctly. However, if they specify the same action twice (e.g.,[Database, Database]), it would only execute once but show 1/2.Also, consider using a
HashSetor deduplicating the actions to prevent potential confusion.rs/src/types.rs (1)
78-97: Consider compiling the Regex once usinglazy_staticorstd::sync::LazyLock.The regex is recompiled on every call to
MWVersion::parse. For a utility that may be called frequently, this is inefficient.Suggested improvement using LazyLock (Rust 1.80+)
+use std::sync::LazyLock; + +static MW_VERSION_RE: LazyLock<Regex> = LazyLock::new(|| { + Regex::new(r"(\d+)\.(\d+)\.(\d+)(?:-([a-zA-Z0-9]+))?").unwrap() +}); + impl MWVersion { pub fn parse(string: &str) -> Option<Self> { - let re = Regex::new(r"(\d+)\.(\d+)\.(\d+)(?:-([a-zA-Z0-9]+))?") - .unwrap(); - - let captures = re.captures(string)?; + let captures = MW_VERSION_RE.captures(string)?;rs/src/exec.rs (1)
142-145: Functional side-effect insidemapis non-idiomatic.Using
.map(|db| cmd.arg(db))for its side effect (mutatingcmd) rather than its return value is unclear. Consider usingif letfor clarity.Proposed refactor
- match database.unwrap_or(DbCommandDatabase::Mw) { - DbCommandDatabase::None => None, - DbCommandDatabase::Mw => config.mw_database.as_deref(), - }.map(|db|cmd.arg(db)); + if let Some(db) = match database.unwrap_or(DbCommandDatabase::Mw) { + DbCommandDatabase::None => None, + DbCommandDatabase::Mw => config.mw_database.as_deref(), + } { + cmd.arg(db); + }
Instead of discarding it.
Ensure that parent folders are also created
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@rs/src/modules/db.rs`:
- Around line 170-199: The code currently reads the entire dump into memory,
doesn't check the DB import process exit status, and should ensure CREATE
DATABASE failures propagate; fix by streaming the dump file instead of fs::read
(open a File and use std::io::copy into the child stdin), verify the CREATE
DATABASE call via the existing run_sql_query invocation (propagate any error
from run_sql_query when calling CREATE DATABASE), and after spawning the
importer from create_db_command check the process exit status (use
process.wait() and assert the ExitStatus is success, returning an anyhow error
with context if not); reference SpinnerSequence, drop_mw_database,
run_sql_query, and create_db_command when making these changes.
- Around line 92-106: The current dump uses create_db_command(...,
DbCommandType::Dump) and calls .output() then writes out.stdout to dump_file,
which buffers the entire dump and doesn't check the process exit status; change
this to spawn the command with stdout piped (use
create_db_command(...).stdin(...).stdout(Stdio::piped()) or equivalent), stream
the child.stdout directly into dump_file using std::io::copy (avoiding buffering
the whole dump), await the child to finish and check child.status.success(), and
return a contextual error if the process fails; keep references to
SpinnerSequence, dump_file, and ensure any error contexts (e.g., "Failed to dump
database!" / "Failed to write dump to file!") are preserved or enhanced with the
process exit code/output.
In `@rs/src/modules/security.rs`:
- Around line 72-75: The code constructs patch_file using the branch-derived
name which may contain '/' and create nested paths; update the logic that builds
name (used in format!("{name}.patch") and patch_file) to sanitize/normalize the
branch name into a safe filename (e.g., replace path separators like '/' and '\'
with '_' or use a filename-sanitization helper) before joining with folder and
passing to Command::new("git") so the generated patch path never contains
directory separators and git --output receives a valid single filename.
- Around line 56-69: The git branch lookup currently assumes Command::new("git")
succeeded and that branch_name is non-empty; update the logic around the
Command::new(...).output() call to check output.status.success() and return/bail
with a clear error if the git command failed (e.g., not a git repo or detached
HEAD), then only parse stdout into branch_name if non-empty (guard against "" so
you don't end up with ".patch"); keep the existing branching logic that assigns
to name when args.use_branch_name is true or when
Regex::new(r"^T[0-9]{4,10}$")?.is_match(branch_name) matches, but if branch_name
is empty or git failed, bail with a descriptive message instructing the user to
provide a patch name or use --use-branch-name.
♻️ Duplicate comments (6)
rs/src/modules/setup_repo.rs (2)
43-47: Misleading error message for gitreview.username.This still references
git-review usernamewhile readingconfig.git_username. Align the message (or use a dedicated field).🔧 Suggested fix
set_git_config( "gitreview.username", - config.git_username.clone().ok_or_else(|| anyhow!("git-review username not set!"))?.as_str(), + config.git_username.clone().ok_or_else(|| anyhow!("Git username not set!"))?.as_str(), &repo_folder )?;
50-53: Checkgit reviewexit status.The command’s exit status is ignored, so setup can silently fail.
🔧 Suggested fix
- Command::new("git") + let status = Command::new("git") .args(["review", "-s", "--verbose"]) .current_dir(repo_folder) .status()?; + if !status.success() { + return Err(anyhow!("git review setup failed!")); + } Ok(())rs/src/utils.rs (2)
46-52: Check git config command exit status.The helper returns
Ok(())even if git config fails.🔧 Suggested fix
- Command::new("git") + let status = Command::new("git") .args(["config", "--local", option, value]) .current_dir(repo_folder) .status() .context("Failed to set git option!")?; + if !status.success() { + anyhow::bail!("git config failed with exit code: {:?}", status.code()); + } Ok(())
35-43: Fix regex capture group for MW version extraction.
c.get(0)returns the entire match ('MW_VERSION', '1.35.0'), butMWVersion::parseexpects only the version number (1.35.0). Use capture group 1 instead.🔧 Suggested fix
- re.captures(contents.as_str()) - .map(|c|c.get(0).unwrap().as_str()) - .and_then(MWVersion::parse) + re.captures(contents.as_str()) + .and_then(|c| c.get(1)) + .map(|m| m.as_str()) + .and_then(MWVersion::parse)rs/src/modules/db.rs (2)
259-272: Add a timeout / max retries for DB readiness polling.This loop can wait forever if the DB never becomes ready.
274-283: Ensure temp dump cleanup on import failure.If
import_dumpfails, the temporary dump is never deleted; add a guard/Drop cleanup.
🧹 Nitpick comments (2)
rs/src/main.rs (1)
97-102: Collapse nested if per clippy.Minor readability cleanup.
♻️ Suggested refactor
- if let Err(e) = config { - if !self.works_globally() { - bail!("The selected module only works inside of an mw-dev-kit environment.\n{e}"); - } - } + if let Err(e) = config && !self.works_globally() { + bail!("The selected module only works inside of an mw-dev-kit environment.\n{e}"); + }rs/src/modules/db.rs (1)
289-303: Don’t silently skip read_dir entry errors.
files.flatten()discards per-entry IO errors, which can hide permission or disk issues and cause incomplete delete/list results. Consider returning aResultand propagating entry errors instead of dropping them.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rs/src/modules/setup_repo.rs`:
- Around line 35-42: The git config call using set_git_config currently passes
an incorrectly quoted value "\"https://gerrit.wikimedia.org/r/\"" which stores
literal quote characters in git config and breaks URL matching; update the
second argument passed to set_git_config to the plain string
"https://gerrit.wikimedia.org/r/" (remove the escaped quotes) so that
set_git_config(..., "https://gerrit.wikimedia.org/r/", &repo_folder) stores the
correct URL pattern; locate the call near the set_git_config invocation that
uses config.gerrit_username and replace the quoted value accordingly.
♻️ Duplicate comments (2)
rs/src/utils.rs (1)
41-43: Regex capture group extraction relies onMWVersion::parseflexibility.Using
c.get(0)passes the entire match (e.g.,'MW_VERSION', '1.42.0') rather than just the version. This works becauseMWVersion::parseuses a regex that can find the version within the larger string. The design is intentional per maintainer's prior response.rs/src/modules/setup_repo.rs (1)
43-48: Error message inconsistency acknowledged via TODO.The TODO at line 43 notes that
git_usernameis used but the error says "git-review username not set!". This was flagged previously and the maintainer indicated it will be addressed later.
🧹 Nitpick comments (3)
rs/src/utils.rs (2)
15-19: Consider usinglet-elsefor more idiomatic Rust.The current pattern works correctly, but
let-elseis cleaner and avoids the.unwrap():♻️ Suggested improvement
- let base_dir = find_base_dir(); - if base_dir.is_none() { - return vec![]; - } - create_docker_compose_command(&base_dir.unwrap()) + let Some(base_dir) = find_base_dir() else { + return vec![]; + }; + create_docker_compose_command(&base_dir)
64-76: Consider adding bounds check innext()to prevent counter overflow display.If
next()is called more thanmaxtimes, the display would show something like[5/4]. For a local dev tool this is minor, but a simple guard could improve UX.♻️ Optional guard
pub fn next(&mut self, text: &str) { if let Some(spinner) = &self.last { spinner.finish(); } + if self.cur >= self.max { + eprintln!("Warning: SpinnerSequence exceeded expected steps"); + } self.cur += 1;rs/src/modules/setup_repo.rs (1)
60-74: Consider consistent success messaging across setup functions.
setup_githubprints "Done!" on success (line 72) whilesetup_gerritdoes not. For a consistent user experience, consider either adding a similar message tosetup_gerritor removing it fromsetup_github.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@rs/src/modules/db.rs`:
- Around line 157-171: drop_mw_database is executing DROP DATABASE under
DbCommandUser::Mw which is inconsistent with import_dump/create database that
use DbCommandUser::Root; change drop_mw_database to invoke run_sql_query with
DbCommandUser::Root instead of DbCommandUser::Mw so database-level DDL uses the
root privilege, keeping the call site (function drop_mw_database) and the rest
of the error handling (anyhow! check, context, status.success() check)
unchanged.
♻️ Duplicate comments (2)
rs/src/modules/db.rs (2)
276-288: Acknowledged: Infinite loop for database readiness is intentional.Based on learnings, the maintainer prefers allowing users to manually interrupt with Ctrl+C rather than implementing programmatic timeouts.
290-298: Acknowledged: Dump preserved on import failure is intentional.Based on learnings, if
import_dumpfails, the temporary dump file is intentionally kept for manual recovery. Consider adding a brief code comment documenting this behavior for future maintainers.
🧹 Nitpick comments (3)
rs/src/modules/db.rs (3)
123-134: Consider adding error context tofs::remove_file.For consistency with other operations and better error messages, consider adding context to the file removal.
♻️ Suggested improvement
- fs::remove_file(&dump_file)?; + fs::remove_file(&dump_file) + .context("Failed to delete dump file")?;
149-152: Consider handling partial deletion failures gracefully.If deletion of one file fails, the loop exits early and remaining files are not deleted. Consider collecting errors and reporting them at the end, or adding context to each deletion.
♻️ Suggested improvement for resilient deletion
+ let mut errors = Vec::new(); for file in dump_files { - fs::remove_file(&file)?; - println!("Deleted {}", file.to_string_lossy()); + match fs::remove_file(&file) { + Ok(_) => println!("Deleted {}", file.to_string_lossy()), + Err(e) => errors.push((file, e)), + } + } + if !errors.is_empty() { + for (file, err) in &errors { + eprintln!("Failed to delete {}: {}", file.to_string_lossy(), err); + } + bail!("Failed to delete {} dump file(s)", errors.len()); }
341-344: Prefer&strover&Stringfor string parameters.Idiomatic Rust prefers
&stras it's more flexible—callers can pass&String,&str, or string slices without conversion.♻️ Suggested improvement
fn get_dump( config: &MWUtilConfig, - name: &String, + name: &str, existence_check: Existence ) -> anyhow::Result<PathBuf> {
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@rs/src/modules/db.rs`:
- Around line 274-282: The readiness probe currently calls run_sql_query(...)
with DbCommandDatabase::Mw which requires the MW database to exist and creates a
circular dependency with import_dump(); change the probe to use
DbCommandDatabase::None so the loop performs a server-level connectivity check
instead of targeting config.mw_database—locate the loop using
spinner.next("Waiting for the database to be ready") and replace the
DbCommandDatabase::Mw argument passed to run_sql_query with
DbCommandDatabase::None (leaving other parameters unchanged) so the readiness
check does not require the MW DB to exist.
In `@rs/src/modules/security.rs`:
- Line 65: Fix the typo in the error message passed to the bail! macro: change
"Current brancch name is empty (detached HEAD?). Please provide --name." to
"Current branch name is empty (detached HEAD?). Please provide --name." so the
bail! invocation (the error string) uses the correct word "branch".
🧹 Nitpick comments (1)
rs/src/modules/security.rs (1)
81-86: Capture stderr for more informative error messages.Using
.status()discards git's stderr output, making failures harder to diagnose. Consider using.output()to include the error details.Proposed fix
- let status = Command::new("git") + let output = Command::new("git") .args(["format-patch", "HEAD^", "--output", patch_file.to_string_lossy().as_ref()]) - .status()?; - if !status.success() { - bail!("Failed to create patch!"); + .output()?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("Failed to create patch: {stderr}"); }
The
rsfolder contains a rewrite of most of the modules, which heavily improves performance, adds new features and fixes a lot of bugs that are present in the python version.The python version still exists (in the
pysubfolder), but will no longer be maintained in the future.Summary by CodeRabbit
New Features
Chores
Maintenance
✏️ Tip: You can customize this high-level summary in your review settings.