Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,5 @@ jobs:
exit 1
fi
- run: mise r lint
- name: check wasm build
run: rustup target add wasm32-wasip1 && cargo build --target wasm32-wasip1 -p usage-cli
1 change: 1 addition & 0 deletions cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ serde_with = "3"
tera = "1"
thiserror = "2"
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
Comment on lines 42 to 44
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Missing blank line before the new [target.…] section makes the TOML harder to read and deviates from the project's existing style (compare with the [target.'cfg(unix)'.dependencies] section below, which has a blank line before it).

Suggested change
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
usage-lib = { workspace = true, features = ["clap", "docs", "unstable_choices_env"] }
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


[target.'cfg(unix)'.dependencies]
Expand Down
22 changes: 13 additions & 9 deletions cli/src/cli/complete_word.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ use std::collections::BTreeMap;
use std::env;
use std::fmt::Debug;
use std::path::{Path, PathBuf};
#[cfg(not(target_arch = "wasm32"))]
use std::process::Command;
use std::sync::Arc;

use clap::Args;
use itertools::Itertools;
use miette::IntoDiagnostic;
use regex::Regex;
use std::sync::LazyLock;
use xx::process::check_status;
use xx::{regex, XXError, XXResult};

use usage::{Spec, SpecArg, SpecCommand, SpecComplete, SpecFlag};

Expand Down Expand Up @@ -290,7 +290,8 @@ impl CompleteWord {
trace!("run: {run}");
let stdout = sh(&run)?;
// trace!("stdout: {stdout}");
let re = regex!(r"[^\\]:");
static COLON_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"[^\\]:").unwrap());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Defining a static LazyLock inside a function is correct, but it's generally more idiomatic and efficient to move these to the module level. This avoids the overhead of the lazy initialization check being scoped within the function logic, especially if called frequently during completions.

let re = &*COLON_RE;
return Ok(stdout
.lines()
.map(|l| {
Expand Down Expand Up @@ -379,10 +380,6 @@ fn zsh_escape(s: &str) -> String {
.replace(']', "\\]")
}

/// Wrap a completion value in single quotes if any character would otherwise
/// be interpreted by the shell. The result is meant to be inserted by
/// `compadd -Q` verbatim, so the user sees consistent single-quote quoting
/// instead of zsh's default mix of backslash and single-quote styles.
fn zsh_shell_quote(s: &str) -> String {
fn safe(c: char) -> bool {
matches!(c,
Comment on lines 383 to 385
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The documentation comments for zsh_shell_quote were accidentally removed. These provide valuable context on why this specific quoting strategy is used for zsh completions and should be restored.

/// Wrap a completion value in single quotes if any character would otherwise
/// be interpreted by the shell. The result is meant to be inserted by
/// compadd -Q verbatim, so the user sees consistent single-quote quoting
/// instead of zsh's default mix of backslash and single-quote styles.
fn zsh_shell_quote(s: &str) -> String {
References
  1. Prefer Rust-side shell quoting (e.g., zsh_shell_quote) over shell-side manual unescaping logic for completion scripts to ensure robustness and maintainability.

Expand All @@ -393,12 +390,14 @@ fn zsh_shell_quote(s: &str) -> String {
if !s.is_empty() && s.chars().all(safe) {
return s.to_string();
}
// Wrap in single quotes; close-open dance escapes any internal apostrophes.
let escaped = s.replace('\'', "'\\''");
format!("'{escaped}'")
Comment on lines 383 to 394
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Doc-comment inadvertently removed

The three-line /// doc-comment for zsh_shell_quote and the inline // Wrap in single quotes… comment inside the body were deleted in this diff, but they are unrelated to the wasm32 changes. The doc-comment explains why the function chooses consistent single-quote quoting over zsh's default mix of styles — useful context for anyone working on completion output later.

}

fn sh(script: &str) -> XXResult<String> {
#[cfg(not(target_arch = "wasm32"))]
fn sh(script: &str) -> xx::XXResult<String> {
use xx::process::check_status;
use xx::XXError;
let output = Command::new("sh")
.arg("-c")
.arg(script)
Expand All @@ -413,3 +412,8 @@ fn sh(script: &str) -> XXResult<String> {
let stdout = String::from_utf8(output.stdout).expect("stdout is not utf-8");
Ok(stdout)
}

#[cfg(target_arch = "wasm32")]
fn sh(_script: &str) -> miette::Result<String> {
Err(miette::miette!("shell execution is not supported on wasm"))
}
Comment on lines 397 to +419
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Inconsistent return types between target variants of sh

The non-wasm32 variant returns xx::XXResult<String> (Result<String, xx::XXError>) while the wasm32 variant returns miette::Result<String> (Result<String, miette::Report>). Both happen to coerce correctly through ? into the caller's miette::Result, but the two arms of the same logical function having different error types can be surprising. For consistency with lib/src/sh.rs — and to make future callers easier — consider giving the wasm32 arm the same outer return-type shape, either by defining a local alias or by aligning on miette::Result in both arms.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

6 changes: 5 additions & 1 deletion cli/src/cli/generate/fig.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,11 @@ impl Fig {
pub fn run(&self) -> miette::Result<()> {
let write = |path: &PathBuf, md: &str| -> miette::Result<()> {
println!("writing to {}", path.display());
xx::file::write(path, format!("{}\n", md.trim()))?;
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent).map_err(|e| miette::miette!("{e}"))?;
}
std::fs::write(path, format!("{}\n", md.trim()))
.map_err(|e| miette::miette!("{e}"))?;
Comment on lines +360 to +363
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of manually mapping the error to a miette report using a string, you can use into_diagnostic() which is more idiomatic in this codebase and preserves the underlying error context.

            if let Some(parent) = path.parent() {
                std::fs::create_dir_all(parent).into_diagnostic()?;
            }
            std::fs::write(path, format!("{}\n", md.trim()))
                .into_diagnostic()?;

Ok(())
};
let spec = generate::file_or_spec(&self.file, &self.spec)?;
Expand Down
6 changes: 5 additions & 1 deletion cli/src/cli/generate/manpage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ impl Manpage {

if let Some(out_file) = &self.out_file {
println!("writing to {}", out_file.display());
xx::file::write(out_file, &manpage)?;
if let Some(parent) = out_file.parent() {
std::fs::create_dir_all(parent).map_err(|e| miette::miette!("{e}"))?;
}
std::fs::write(out_file, &manpage)
.map_err(|e| miette::miette!("{e}"))?;
} else {
print!("{}", manpage);
}
Expand Down
8 changes: 6 additions & 2 deletions cli/src/cli/generate/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,17 @@ impl Markdown {
pub fn run(&self) -> miette::Result<()> {
let write = |path: &PathBuf, md: &str| -> miette::Result<()> {
println!("writing to {}", path.display());
xx::file::write(
if let Some(parent) = path.parent() {
std::fs::create_dir_all(parent).map_err(|e| miette::miette!("{e}"))?;
}
std::fs::write(
path,
format!(
"<!-- @generated by usage-cli from usage spec -->\n{}\n",
md.trim()
),
)?;
)
.map_err(|e| miette::miette!("{e}"))?;
Ok(())
};
let spec = parse_file_or_stdin(&self.file)?;
Expand Down
1 change: 1 addition & 0 deletions cli/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#[macro_use]
extern crate log;
extern crate miette;
#[cfg(not(target_arch = "wasm32"))]
extern crate xx;

use miette::Result;
Expand Down
1 change: 1 addition & 0 deletions lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ strum = { version = "0.28", features = ["derive"] }
tera = { version = "1", optional = true }
thiserror = "2"
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
Comment on lines 39 to 41
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Same missing blank-line issue as in cli/Cargo.toml — the new [target.…] section header is run directly into the last [dependencies] entry without a blank line separator.

Suggested change
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"
versions = "7"
[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
xx = "2"


[features]
Expand Down
12 changes: 8 additions & 4 deletions lib/src/docs/markdown/renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use crate::error::UsageErr;
use itertools::Itertools;
use serde::Serialize;
use std::collections::HashMap;
use xx::regex;
use regex::Regex;
use std::sync::LazyLock;

#[derive(Debug, Clone)]
pub struct MarkdownRenderer {
Expand Down Expand Up @@ -88,7 +89,10 @@ impl MarkdownRenderer {
return line.to_string();
}
// replace '<' with '&lt;' but not inside code blocks
xx::regex!(r"(`[^`]*`)|(<)")
{
static RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"(`[^`]*`)|(<)").unwrap());
&RE
}
Comment on lines +93 to +95
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Defining a static regex inside a closure that is executed for every line of a markdown file is inefficient. While LazyLock ensures it's only initialized once, the check still occurs on every iteration. Move this static to the module level.

.replace_all(line, |caps: &regex::Captures| {
if caps.get(1).is_some() {
caps.get(1).unwrap().as_str().to_string()
Expand All @@ -102,8 +106,8 @@ impl MarkdownRenderer {
Ok(value.into())
},
);
let path_re =
regex!(r"https://(github.com/[^/]+/[^/]+|gitlab.com/[^/]+/[^/]+/-)/blob/[^/]+/");
static PATH_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"https://(github.com/[^/]+/[^/]+|gitlab.com/[^/]+/[^/]+/-)/blob/[^/]+/").unwrap());
let path_re = &*PATH_RE;
tera.register_function("source_code_link", |args: &HashMap<String, tera::Value>| {
let spec = args.get("spec").unwrap().as_object().unwrap();
let cmd = args.get("cmd").unwrap().as_object().unwrap();
Expand Down
1 change: 1 addition & 0 deletions lib/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ pub enum UsageErr {
#[diagnostic(transparent)]
KdlError(#[from] kdl::KdlError),

#[cfg(not(target_arch = "wasm32"))]
#[error(transparent)]
#[diagnostic(transparent)]
XXError(#[from] xx::error::XXError),
Expand Down
12 changes: 12 additions & 0 deletions lib/src/sh.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
#[cfg(not(target_arch = "wasm32"))]
use std::process::Command;
#[cfg(not(target_arch = "wasm32"))]
use xx::process::check_status;
#[cfg(not(target_arch = "wasm32"))]
use xx::{XXError, XXResult};

#[cfg(not(target_arch = "wasm32"))]
pub(crate) fn sh(script: &str) -> XXResult<String> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The return type of the non-wasm sh implementation (XXResult<String>) differs from the wasm implementation (Result<String, UsageErr>). For consistency and to avoid issues in target-agnostic callers, both should return Result<String, UsageErr>. Since UsageErr already implements From<XXError>, this change is straightforward.

Suggested change
pub(crate) fn sh(script: &str) -> XXResult<String> {
pub(crate) fn sh(script: &str) -> Result<String, crate::error::UsageErr> {

#[cfg(unix)]
let (shell, flag) = ("sh", "-c");
Expand All @@ -23,3 +27,11 @@ pub(crate) fn sh(script: &str) -> XXResult<String> {
let stdout = String::from_utf8(output.stdout).expect("stdout is not utf-8");
Ok(stdout)
}

#[cfg(target_arch = "wasm32")]
pub(crate) fn sh(_script: &str) -> std::result::Result<String, crate::error::UsageErr> {
Err(crate::error::UsageErr::IO(std::io::Error::new(
std::io::ErrorKind::Unsupported,
"shell execution is not supported on wasm",
)))
}
17 changes: 10 additions & 7 deletions lib/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ use std::fmt::{Display, Formatter};
use std::iter::once;
use std::path::Path;
use std::str::FromStr;
use xx::file;
use regex::Regex;
use std::sync::LazyLock;

use crate::error::UsageErr;
use crate::spec::cmd::{SpecCommand, SpecExample};
Expand Down Expand Up @@ -102,7 +103,7 @@ impl Spec {
/// If `bin` is not specified in the spec, it defaults to the filename.
#[must_use = "parsing result should be used"]
pub fn parse_script(file: &Path) -> Result<Spec, UsageErr> {
let raw = extract_usage_from_comments(&file::read_to_string(file)?);
let raw = extract_usage_from_comments(&std::fs::read_to_string(file)?);
let ctx = ParsingContext::new(file, &raw);
let mut spec = Self::parse(&ctx, &raw)?;
if spec.bin.is_empty() {
Expand Down Expand Up @@ -303,11 +304,11 @@ fn check_usage_version(version: &str) {
}

fn split_script(file: &Path) -> Result<String, UsageErr> {
let full = file::read_to_string(file)?;
let full = std::fs::read_to_string(file)?;
// If file has a shebang and USAGE comments, extract the spec from comments
if full.starts_with("#!") {
let usage_regex = xx::regex!(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])");
if full.lines().any(|l| usage_regex.is_match(l)) {
static USAGE_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])").unwrap());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Consider moving this static regex to the module level to improve readability and ensure initialization happens outside of the core logic of split_script.

if full.lines().any(|l| USAGE_RE.is_match(l)) {
return Ok(extract_usage_from_comments(&full));
}
}
Expand All @@ -316,8 +317,10 @@ fn split_script(file: &Path) -> Result<String, UsageErr> {
}

fn extract_usage_from_comments(full: &str) -> String {
let usage_regex = xx::regex!(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])(.*)$");
let blank_comment_regex = xx::regex!(r"^(?:#|//|::)\s*$");
static USAGE_CAPTURE_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^(?:#|//|::)(?:USAGE| ?\[USAGE\])(.*)$").unwrap());
static BLANK_COMMENT_RE: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^(?:#|//|::)\s*$").unwrap());
let usage_regex = &*USAGE_CAPTURE_RE;
let blank_comment_regex = &*BLANK_COMMENT_RE;
let mut usage = vec![];
let mut found = false;
for line in full.lines() {
Expand Down