Skip to content
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

Improve consistency of error message wordings #722

Merged
merged 1 commit into from
Nov 7, 2023
Merged
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
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,
);
edmorley marked this conversation as resolved.
Show resolved Hide resolved

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