Skip to content

Commit

Permalink
Improve consistency of error message wordings
Browse files Browse the repository at this point in the history
* Use contractions per Simplified Technical English guidelines
  (e.g. "Couldn't" instead of "Could not").
* Prefer using past tense for potentially transient failures
  (e.g. "Couldn't read buildpack.toml" rather than "Can't").
* Use "I/O error" instead of "IO error" or "IOError"
* Use display representations when including underlying error
  messages in the current error message where possible (ie: For
  most errors apart from `io::Error`). This also means preferring
`panic!` over `.expect()`, since the latter uses the debug
  representation instead of display.
* Use "Error ..." as the prefix for all top level error message
  (i.e. at the `panic!` or `.expect()` call sites).
* Use `#[error(transparent)]` instead of `#[error("{0}")]` per:
  #720 (comment)
  • Loading branch information
edmorley committed Nov 7, 2023
1 parent cfd3c3d commit 69f6026
Show file tree
Hide file tree
Showing 28 changed files with 82 additions and 71 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- Improved the consistency of all user-facing libcnb.rs error message wordings. ([#722](https://github.com/heroku/libcnb.rs/pull/722))

### Fixed

- `libcnb-test`:
Expand Down
2 changes: 1 addition & 1 deletion examples/ruby-sample/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub(crate) fn sha256_checksum(path: impl AsRef<Path>) -> Result<String, std::io:
fs::read(path).map(|bytes| format!("{:x}", sha2::Sha256::digest(bytes)))
}

/// Helper to run very simple commands where we just need to handle IO errors and non-zero exit
/// Helper to run very simple commands where we just need to handle I/O errors and non-zero exit
/// codes. Not very useful in complex scenarios, but can cut down the amount of code in simple
/// cases.
pub(crate) fn run_simple_command<E, F: FnOnce(std::io::Error) -> E, F2: FnOnce(ExitStatus) -> E>(
Expand Down
2 changes: 1 addition & 1 deletion libcnb-cargo/src/package/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub(crate) fn execute(args: &PackageArgs) -> Result<(), Error> {
CrossCompileAssistance::Configuration { cargo_env } => cargo_env,
CrossCompileAssistance::NoAssistance => {
eprintln!(
"Could not determine automatic cross-compile settings for target triple {}.",
"Couldn't determine automatic cross-compile settings for target triple {}.",
args.target
);
eprintln!("This is not an error, but without proper cross-compile settings in your Cargo manifest and locally installed toolchains, compilation might fail.");
Expand Down
6 changes: 3 additions & 3 deletions libcnb-common/src/toml_file.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{fs, path::Path};
/// An error that occurred during reading or writing a TOML file.
#[derive(thiserror::Error, Debug)]
pub enum TomlFileError {
#[error("IO error while reading/writing TOML file: {0}")]
#[error("I/O error while reading/writing TOML file: {0}")]
IoError(#[from] std::io::Error),

#[error("TOML deserialization error while reading TOML file: {0}")]
Expand All @@ -18,7 +18,7 @@ pub enum TomlFileError {
///
/// # Errors
///
/// Will return `Err` if the file could not be written or the value could not be serialized as a TOML string.
/// Will return `Err` if the file couldn't be written or the value couldn't be serialized as a TOML string.
pub fn write_toml_file(
value: &impl Serialize,
path: impl AsRef<Path>,
Expand All @@ -32,7 +32,7 @@ pub fn write_toml_file(
///
/// # Errors
///
/// Will return `Err` if the file could not be read or its contents could not be deserialized.
/// Will return `Err` if the file couldn't be read or its contents couldn't be deserialized.
pub fn read_toml_file<A: DeserializeOwned>(path: impl AsRef<Path>) -> Result<A, TomlFileError> {
let contents = fs::read_to_string(path)?;
Ok(toml::from_str(&contents)?)
Expand Down
2 changes: 1 addition & 1 deletion libcnb-data/src/build_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ impl Require {
Ok(())
} else {
Err(toml::ser::Error::custom(String::from(
"Could not be serialized as a TOML Table.",
"Couldn't be serialized as a TOML Table.",
)))
}
}
Expand Down
6 changes: 5 additions & 1 deletion libcnb-data/src/buildpack_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ impl Entry {
where
T: Deserialize<'de>,
{
// All toml `Value`s have `try_into()` which converts them to a `T` if `Deserialize` and `Deserializer` is implemented for `T`. Sadly, the `Table` type we use in `Entry` is not a `Value` so we need to make it one by wrapping it. We cannot wrap directly in `Entry` since that would allow users to put non-table TOML values as metadata. As outlined earlier, we can't get around the clone since we're only borrowing the metadata.
// All toml `Value`s have `try_into()` which converts them to a `T` if `Deserialize` and
// `Deserializer` is implemented for `T`. Sadly, the `Table` type we use in `Entry` is not
// a `Value` so we need to make it one by wrapping it. We can't wrap directly in `Entry`
// since that would allow users to put non-table TOML values as metadata. As outlined
// earlier, we can't get around the clone since we're only borrowing the metadata.
toml::Value::Table(self.metadata.clone()).try_into()
}
}
Expand Down
6 changes: 3 additions & 3 deletions libcnb-package/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::process::{Command, ExitStatus};
///
/// # Errors
///
/// Will return `Err` if any build did not finish successfully, the configuration cannot be
/// Will return `Err` if any build did not finish successfully, the configuration can't be
/// read or the configured main buildpack binary does not exist.
pub fn build_buildpack_binaries(
project_path: impl AsRef<Path>,
Expand Down Expand Up @@ -167,7 +167,7 @@ pub struct BuildpackBinaries {

#[derive(thiserror::Error, Debug)]
pub enum BuildError {
#[error("Error while running Cargo build process: {0}")]
#[error("I/O error while running Cargo build process: {0}")]
CargoProcessIoError(#[source] std::io::Error),
#[error("Cargo unexpectedly exited with status {0}")]
UnexpectedCargoExitStatus(ExitStatus),
Expand All @@ -179,6 +179,6 @@ pub enum BuildBinariesError {
CannotDetermineBuildpackCargoTargetName(#[source] DetermineBuildpackCargoTargetNameError),
#[error("Failed to build binary target {0}: {1}")]
BuildError(String, #[source] BuildError),
#[error("Binary target {0} could not be found")]
#[error("Binary target {0} couldn't be found")]
MissingBuildpackTarget(String),
}
6 changes: 3 additions & 3 deletions libcnb-package/src/buildpack_dependency_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use std::path::{Path, PathBuf};
/// # Errors
///
/// Returns `Err` if a buildpack declares an invalid dependency, has an invalid buildpack.toml or
/// package.toml or an IO error occurred while traversing the given directory.
/// package.toml or an I/O error occurred while traversing the given directory.
pub fn build_libcnb_buildpacks_dependency_graph(
cargo_workspace_root: &Path,
) -> Result<Graph<BuildpackDependencyGraphNode, ()>, BuildBuildpackDependencyGraphError> {
Expand Down Expand Up @@ -86,9 +86,9 @@ fn build_libcnb_buildpack_dependency_graph_node(
pub enum BuildBuildpackDependencyGraphError {
#[error("Error while finding buildpack directories: {0}")]
FindBuildpackDirectories(ignore::Error),
#[error("Cannot read buildpack descriptor: {0}")]
#[error("Couldn't read buildpack.toml: {0}")]
ReadBuildpackDescriptorError(TomlFileError),
#[error("Cannot read package descriptor: {0}")]
#[error("Couldn't read package.toml: {0}")]
ReadPackageDescriptorError(TomlFileError),
#[error("Dependency uses an invalid buildpack id: {0}")]
InvalidDependencyBuildpackId(BuildpackIdError),
Expand Down
2 changes: 1 addition & 1 deletion libcnb-package/src/cross_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use which::which;
/// Provides assistance for cross-compiling from the user's host platform to the desired target platform.
///
/// This function will not install required toolchains, linkers or compilers automatically. It will
/// look for the required tools and returns a human-readable help text if they cannot be found or
/// look for the required tools and returns a human-readable help text if they can't be found or
/// any other issue has been detected.
pub fn cross_compile_assistance(target_triple: impl AsRef<str>) -> CrossCompileAssistance {
// Background: https://omarkhawaja.com/cross-compiling-rust-from-macos-to-linux/
Expand Down
2 changes: 1 addition & 1 deletion libcnb-package/src/dependency_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ where
/// # Errors
///
/// Will return an `Err` if the graph contains references to missing dependencies or the
/// dependencies of a [`DependencyNode`] could not be gathered.
/// dependencies of a [`DependencyNode`] couldn't be gathered.
pub fn create_dependency_graph<T, I, E>(
nodes: Vec<T>,
) -> Result<Graph<T, ()>, CreateDependencyGraphError<I, E>>
Expand Down
8 changes: 4 additions & 4 deletions libcnb-package/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub enum CargoProfile {
///
/// # Errors
///
/// Will return `Err` if the buildpack directory could not be assembled.
/// Will return `Err` if the buildpack directory couldn't be assembled.
pub fn assemble_buildpack_directory(
destination_path: impl AsRef<Path>,
buildpack_descriptor_path: impl AsRef<Path>,
Expand Down Expand Up @@ -132,7 +132,7 @@ pub fn find_buildpack_dirs(start_dir: &Path) -> Result<Vec<PathBuf>, ignore::Err
///
/// # Errors
///
/// Will return an `Err` if the root workspace directory cannot be located due to:
/// Will return an `Err` if the root workspace directory can't be located due to:
/// - no `CARGO` environment variable with the path to the `cargo` binary
/// - executing this function with a directory that is not within a Cargo project
/// - any other file or system error that might occur
Expand Down Expand Up @@ -167,13 +167,13 @@ pub fn find_cargo_workspace_root_dir(

#[derive(thiserror::Error, Debug)]
pub enum FindCargoWorkspaceRootError {
#[error("Cannot get value of CARGO environment variable: {0}")]
#[error("Couldn't get value of CARGO environment variable: {0}")]
GetCargoEnv(#[source] std::env::VarError),
#[error("Error while spawning Cargo process: {0}")]
SpawnCommand(#[source] std::io::Error),
#[error("Unexpected Cargo exit status ({}) while attempting to read workspace root", exit_code_or_unknown(*.0))]
CommandFailure(std::process::ExitStatus),
#[error("Could not locate a Cargo workspace within {0} or its parent directories")]
#[error("Couldn't locate a Cargo workspace within {0} or its parent directories")]
GetParentDirectory(PathBuf),
}

Expand Down
14 changes: 7 additions & 7 deletions libcnb-package/src/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ pub fn package_buildpack(

#[derive(thiserror::Error, Debug)]
pub enum PackageBuildpackError {
#[error("{0}")]
#[error(transparent)]
PackageCompositeBuildpackError(PackageCompositeBuildpackError),
#[error("{0}")]
#[error(transparent)]
PackageLibcnbBuildpackError(PackageLibcnbBuildpackError),
#[error("Buildpack is not supported to be packaged")]
UnsupportedBuildpack,
Expand Down Expand Up @@ -95,7 +95,7 @@ pub fn package_libcnb_buildpack(
pub enum PackageLibcnbBuildpackError {
#[error("Assembling buildpack directory failed: {0}")]
AssembleBuildpackDirectory(std::io::Error),
#[error("IO error while writing package descriptor: {0}")]
#[error("Couldn't write package.toml: {0}")]
WritePackageDescriptor(std::io::Error),
#[error("Building buildpack binaries failed: {0}")]
BuildBinariesError(crate::build::BuildBinariesError),
Expand All @@ -115,7 +115,7 @@ pub enum PackageLibcnbBuildpackError {
/// # Errors
///
/// Returns `Err` if a `libcnb:` URI refers to a buildpack not in `buildpack_paths` or packaging
/// otherwise failed (i.e. IO errors).
/// otherwise failed (i.e. I/O errors).
pub fn package_composite_buildpack(
buildpack_directory: &Path,
destination: &Path,
Expand Down Expand Up @@ -150,12 +150,12 @@ pub fn package_composite_buildpack(

#[derive(thiserror::Error, Debug)]
pub enum PackageCompositeBuildpackError {
#[error("Could not copy buildpack.toml: {0}")]
#[error("Couldn't copy buildpack.toml: {0}")]
CouldNotCopyBuildpackToml(std::io::Error),
#[error("Could not read package.toml: {0}")]
#[error("Couldn't read package.toml: {0}")]
CouldNotReadPackageDescriptor(TomlFileError),
#[error("Error while normalizing package.toml: {0}")]
NormalizePackageDescriptorError(NormalizePackageDescriptorError),
#[error("Could not write package descriptor: {0}")]
#[error("Couldn't write package.toml: {0}")]
CouldNotWritePackageDescriptor(TomlFileError),
}
8 changes: 4 additions & 4 deletions libcnb-package/src/package_descriptor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ pub(crate) fn normalize_package_descriptor(

#[derive(thiserror::Error, Debug)]
pub enum NormalizePackageDescriptorError {
#[error("{0}")]
ReplaceLibcnbUriError(#[source] ReplaceLibcnbUriError),
#[error("{0}")]
PackageDescriptorDependencyError(#[source] PackageDescriptorDependencyError),
#[error(transparent)]
ReplaceLibcnbUriError(ReplaceLibcnbUriError),
#[error(transparent)]
PackageDescriptorDependencyError(PackageDescriptorDependencyError),
}

fn replace_libcnb_uris(
Expand Down
2 changes: 1 addition & 1 deletion libcnb-package/src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::path::{Component, Path, PathBuf};
///
/// # Errors
///
/// Returns `Err` if an IO error occurred during the size calculation.
/// Returns `Err` if an I/O error occurred during the size calculation.
pub fn calculate_dir_size(path: impl AsRef<Path>) -> std::io::Result<u64> {
let mut size_in_bytes = 0;

Expand Down
6 changes: 3 additions & 3 deletions libcnb-proc-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub fn verify_regex(input: TokenStream) -> TokenStream {
}
Err(err) => syn::Error::new(
input.regex.span(),
format!("Could not compile regular expression: {err}"),
format!("Couldn't compile regular expression: {err}"),
)
.to_compile_error(),
};
Expand Down Expand Up @@ -110,12 +110,12 @@ pub fn verify_bin_target_exists(input: TokenStream) -> TokenStream {
}
} else {
quote! {
compile_error!("Cannot read root package for this crate!")
compile_error!("Couldn't read root package for this crate!")
}
}
} else {
quote! {
compile_error!("Cannot read Cargo metadata!")
compile_error!("Couldn't read Cargo metadata!")
}
};

Expand Down
4 changes: 3 additions & 1 deletion libcnb-test/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ pub(crate) fn copy_app(app_dir: impl AsRef<Path>) -> Result<AppDir, PrepareAppEr
})
}

#[derive(Debug)]
#[derive(thiserror::Error, Debug)]
pub enum PrepareAppError {
#[error("Couldn't create temporary directory: {0}")]
CreateTempDirError(std::io::Error),
#[error("Couldn't copy directory: {0}")]
CopyAppError(fs_extra::error::Error),
}

Expand Down
2 changes: 1 addition & 1 deletion libcnb-test/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ pub(crate) enum PackageBuildpackError {
BuildpackDescriptorNotFound(PathBuf),
#[error("Couldn't find a buildpack with ID '{0}' in the workspace at {1}")]
BuildpackIdNotFound(BuildpackId, PathBuf),
#[error("I/O error creating directory {0}: {1}")]
#[error("Couldn't create directory {0}: {1}")]
CannotCreateDirectory(PathBuf, io::Error),
#[error("Couldn't read buildpack.toml: {0}")]
CannotReadBuildpackDescriptor(TomlFileError),
Expand Down
2 changes: 1 addition & 1 deletion libcnb-test/src/container_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl ContainerContext {
.stdout
.trim()
.parse()
.expect("Couldn't parse `docker port` output"),
.unwrap_or_else(|error| panic!("Error parsing `docker port` output: {error}")),
Err(CommandError::NonZeroExitCode { log_output, .. }) => {
panic!(
"Error obtaining container port mapping:\n{}\nThis normally means that the container crashed. Container logs:\n\n{}",
Expand Down
4 changes: 2 additions & 2 deletions libcnb-test/src/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<'a> TestContext<'a> {
///
/// # Panics
///
/// Panics if there was an error starting the container, such as when the specified entrypoint/command cannot be found.
/// Panics if there was an error starting the container, such as when the specified entrypoint/command can't be found.
///
/// Note: Does not panic if the container exits after starting (including if it crashes and exits non-zero).
pub fn start_container<C: Borrow<ContainerConfig>, F: FnOnce(ContainerContext)>(
Expand Down Expand Up @@ -223,7 +223,7 @@ impl<'a> TestContext<'a> {
/// Panics if there was an error creating the temporary directory used to store the
/// SBOM files, or if the Pack CLI command used to download the SBOM files failed.
pub fn download_sbom_files<R, F: Fn(SbomFiles) -> R>(&self, f: F) -> R {
let temp_dir = tempdir().expect("Could not create temporary directory for SBOM files");
let temp_dir = tempdir().expect("Couldn't create temporary directory for SBOM files");

let mut command = PackSbomDownloadCommand::new(&self.image_name);
command.output_dir(temp_dir.path());
Expand Down
11 changes: 6 additions & 5 deletions libcnb-test/src/test_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,10 @@ impl TestRunner {
) {
let config = config.borrow();

let cargo_manifest_dir = env::var("CARGO_MANIFEST_DIR")
.map(PathBuf::from)
.expect("Could not determine Cargo manifest directory");
let cargo_manifest_dir = env::var("CARGO_MANIFEST_DIR").map_or_else(
|error| panic!("Error determining Cargo manifest directory: {error}"),
PathBuf::from,
);

let app_dir = {
let normalized_app_dir_path = if config.app_dir.is_relative() {
Expand All @@ -80,7 +81,7 @@ impl TestRunner {
// preprocessor. Skip app copying if no changes to the app will be made.
if let Some(app_dir_preprocessor) = &config.app_dir_preprocessor {
let temporary_app_dir = app::copy_app(&normalized_app_dir_path)
.expect("Could not copy app to temporary location");
.expect("Error copying app fixture to temporary location");

(app_dir_preprocessor)(temporary_app_dir.as_path().to_owned());

Expand All @@ -91,7 +92,7 @@ impl TestRunner {
};

let buildpacks_target_dir =
tempdir().expect("Could not create a temporary directory for compiled buildpacks");
tempdir().expect("Error creating temporary directory for compiled buildpacks");

let mut pack_command = PackBuildCommand::new(&config.builder_name, &app_dir, &image_name);

Expand Down
2 changes: 1 addition & 1 deletion libcnb-test/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ fn packaging_failure_invalid_buildpack_toml() {
#[test]
#[ignore = "integration test"]
#[should_panic(
expected = "Error packaging buildpack 'libcnb-test/composite-missing-package-toml': Could not read package.toml: IO error while reading/writing TOML file: No such file or directory (os error 2)"
expected = "Error packaging buildpack 'libcnb-test/composite-missing-package-toml': Couldn't read package.toml: I/O error while reading/writing TOML file: No such file or directory (os error 2)"
)]
fn packaging_failure_composite_buildpack_missing_package_toml() {
TestRunner::default().build(
Expand Down
Loading

0 comments on commit 69f6026

Please sign in to comment.