diff --git a/docs/src/commands/buffrs-lint.md b/docs/src/commands/buffrs-lint.md index 926d323d..1a302291 100644 --- a/docs/src/commands/buffrs-lint.md +++ b/docs/src/commands/buffrs-lint.md @@ -5,7 +5,14 @@ Rules](../reference/protocol-buffer-rules.md)) ### Synopsis -`buffrs lint` +`buffrs lint [--deny warnings | --allow warnings]` + +### Exit status + +Every violation is printed. The command exits with a non-zero status when any +error-level violation is reported. Warning-level violations are printed and do +not change the exit status unless `--deny warnings` is passed. `--allow +warnings` selects the default, which reports warnings without failing. ### Description diff --git a/src/command.rs b/src/command.rs index ec85dc6f..27ddcd32 100644 --- a/src/command.rs +++ b/src/command.rs @@ -397,8 +397,11 @@ pub async fn list() -> miette::Result<()> { } /// Parses current package and validates rules. +/// +/// Every violation is printed. The command fails when an error-level violation +/// is present, and also when `deny_warnings` is set and a warning is present. #[cfg(feature = "validation")] -pub async fn lint() -> miette::Result<()> { +pub async fn lint(deny_warnings: bool) -> miette::Result<()> { let manifest_path = PathBuf::from(MANIFEST_FILE); let manifest = Manifest::require_package_manifest(&manifest_path).await?; let store = PackageStore::current().await?; @@ -409,12 +412,20 @@ pub async fn lint() -> miette::Result<()> { store.populate(&pkg).await?; let violations = store.validate(&pkg).await?; + let fatal = violations + .iter() + .filter(|violation| violation.level.causes_lint_failure(deny_warnings)) + .count(); violations .into_iter() .map(miette::Report::new) .for_each(|r| eprintln!("{r:?}")); + if fatal > 0 { + bail!("found {fatal} rule violation(s)"); + } + Ok(()) } diff --git a/src/main.rs b/src/main.rs index 1f39465f..c2733882 100644 --- a/src/main.rs +++ b/src/main.rs @@ -70,7 +70,19 @@ enum Command { }, /// Check rule violations for this package. - Lint, + Lint { + /// Fail the command when violations of this severity are reported. + /// + /// Error-level violations always fail the command. Warnings are printed + /// either way, and fail the command only when denied. + #[arg(long, value_name = "LEVEL", conflicts_with = "allow")] + deny: Option, + /// Do not fail the command when violations of this severity are reported. + /// + /// This is the default for warnings. + #[arg(long, value_name = "LEVEL", conflicts_with = "deny")] + allow: Option, + }, /// Adds dependencies to a manifest file Add { @@ -272,10 +284,14 @@ async fn main() -> miette::Result<()> { .wrap_err(miette!( "failed to publish `{package}` to `{registry}:{repository}`", )), - Command::Lint => command::lint().await.wrap_err(miette!( - "failed to lint protocol buffers in `{}`", - PackageStore::PROTO_PATH - )), + Command::Lint { deny, allow: _ } => { + let deny_warnings = matches!(deny, Some(LintSeverity::Warnings)); + + command::lint(deny_warnings).await.wrap_err(miette!( + "failed to lint protocol buffers in `{}`", + PackageStore::PROTO_PATH + )) + } Command::Install { preserve_local_mtime, offline, @@ -303,6 +319,12 @@ async fn main() -> miette::Result<()> { } } +#[derive(Copy, Clone, Debug, PartialEq, Eq, clap::ValueEnum)] +enum LintSeverity { + /// Warning-level rule violations. + Warnings, +} + fn infer_package_type(lib: bool, api: bool) -> Option { if lib { Some(PackageType::Lib) diff --git a/src/validation/rules/package_hierarchy.rs b/src/validation/rules/package_hierarchy.rs index 6db35477..a57c40e4 100644 --- a/src/validation/rules/package_hierarchy.rs +++ b/src/validation/rules/package_hierarchy.rs @@ -48,7 +48,7 @@ impl Rule for PackageHierarchy { for (expected, component) in expected_components.iter().zip(components) { if component != *expected { return Some(self.to_violation(violation::Message { - message: format!("expected file {} to live in {}.", path.to_string_lossy(), &expected_path), + message: format!("expected file {} to live in {}.", path.to_string_lossy(), expected_path), help: "Package names should be mirrored in folder structure, eg mypackage.subpackage should live in proto/subpackage.".into(), })); } diff --git a/src/validation/violation.rs b/src/validation/violation.rs index 99cbe9cf..59b9de7a 100644 --- a/src/validation/violation.rs +++ b/src/validation/violation.rs @@ -28,6 +28,20 @@ pub enum Level { Error, } +impl Level { + /// Whether `buffrs lint` should exit with a failure for this severity. + /// + /// Errors always fail the command. Warnings fail only when `deny_warnings` + /// is set. Informational violations are reported and never fail the command. + pub fn causes_lint_failure(self, deny_warnings: bool) -> bool { + match self { + Self::Error => true, + Self::Warning => deny_warnings, + Self::Info => false, + } + } +} + /// Location of violation. #[derive(Default, PartialEq, Clone, Eq, Debug)] pub struct Location { @@ -73,7 +87,7 @@ impl std::error::Error for Violation {} impl Display for Violation { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "{}", &self.info) + write!(f, "{}", self.info) } } @@ -134,3 +148,26 @@ impl SourceCode for Location { ))) } } + +#[cfg(test)] +mod tests { + use super::Level; + + #[test] + fn errors_fail_lint_regardless_of_warning_policy() { + assert!(Level::Error.causes_lint_failure(false)); + assert!(Level::Error.causes_lint_failure(true)); + } + + #[test] + fn warnings_fail_lint_only_when_denied() { + assert!(!Level::Warning.causes_lint_failure(false)); + assert!(Level::Warning.causes_lint_failure(true)); + } + + #[test] + fn info_never_fails_lint() { + assert!(!Level::Info.causes_lint_failure(false)); + assert!(!Level::Info.causes_lint_failure(true)); + } +} diff --git a/tests/cmd/lint/mod.rs b/tests/cmd/lint/mod.rs index 911e4c61..eede2699 100644 --- a/tests/cmd/lint/mod.rs +++ b/tests/cmd/lint/mod.rs @@ -12,3 +12,74 @@ fn fixture() { .stdout(include_str!("stdout.log")) .stderr(include_str!("stderr.log")); } + +#[test] +fn exits_with_failure_when_a_rule_is_violated() { + let vfs = VirtualFileSystem::empty(); + let root = vfs.root(); + + crate::cli!() + .args(["init", "--lib", "my-protos"]) + .current_dir(&root) + .assert() + .success(); + + std::fs::write( + root.join("proto/my_protos.proto"), + "syntax = \"proto3\";\npackage wrongpkg;\nmessage Foo { string bar = 1; }\n", + ) + .unwrap(); + + let output = crate::cli!() + .arg("lint") + .current_dir(&root) + .assert() + .failure(); + + let stderr = String::from_utf8_lossy(&output.get_output().stderr); + assert!( + stderr.contains("wrongpkg"), + "expected the package-name violation in stderr, got:\n{stderr}" + ); + assert!( + stderr.contains("found 1 rule violation(s)"), + "expected a failing lint summary in stderr, got:\n{stderr}" + ); +} + +#[test] +fn allow_warnings_still_fails_on_errors() { + let vfs = VirtualFileSystem::empty(); + let root = vfs.root(); + + crate::cli!() + .args(["init", "--lib", "my-protos"]) + .current_dir(&root) + .assert() + .success(); + + std::fs::write( + root.join("proto/my_protos.proto"), + "syntax = \"proto3\";\npackage wrongpkg;\nmessage Foo { string bar = 1; }\n", + ) + .unwrap(); + + crate::cli!() + .args(["lint", "--allow", "warnings"]) + .current_dir(&root) + .assert() + .failure(); +} + +#[test] +fn deny_warnings_keeps_a_clean_package_successful() { + let vfs = VirtualFileSystem::copy(crate::parent_directory!().join("in")); + + crate::cli!() + .args(["lint", "--deny", "warnings"]) + .current_dir(vfs.root()) + .assert() + .success() + .stdout(include_str!("stdout.log")) + .stderr(include_str!("stderr.log")); +}