Skip to content

Commit

Permalink
Remove unneeded struct
Browse files Browse the repository at this point in the history
The `Detail` enum was somewhat confusing as there's a separate concept of a "detail" which means being formatted in parens. I changed it to `raw` which is closer to what it is and more generally applicable.
  • Loading branch information
schneems committed Aug 8, 2023
1 parent 49c30a8 commit 6cbe132
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 56 deletions.
48 changes: 17 additions & 31 deletions buildpacks/ruby/src/user_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub(crate) fn on_error(err: libcnb::Error<RubyBuildpackError>) {
match cause(err) {
Cause::OurError(error) => log_ruby_error(error),
Cause::FrameworkError(error) => Announcement::error()
.detail(attn::Detail::Raw(error.to_string()))
.raw(&error)
.header(
"heroku/buildpack-ruby internal buildpack error"
).body(formatdoc! {"
Expand Down Expand Up @@ -84,9 +84,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
RubyBuildpackError::CannotDetectRakeTasks(error) => {
let local_debug_cmd = local_command_debug(&error);
Announcement::error()
.detail({
attn::Detail::Raw(build_output::fmt::cmd_error(&error))
})
.raw(&build_output::fmt::cmd_error(&error))
.header(
"Error detecting rake tasks"
)
Expand All @@ -101,15 +99,11 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
.print();
},
RubyBuildpackError::BundleListError(error_diagnostics) => Announcement::error()
.detail({
attn::Detail::Raw(
build_output::fmt::cmd_error(&error_diagnostics.error)
)
})
.detail({
attn::Detail::Raw(
build_output::fmt::cmd_diagnostics(&error_diagnostics.diagnostics)
)
.raw(
&build_output::fmt::cmd_error(&error_diagnostics.error)
)
.raw({
&build_output::fmt::cmd_diagnostics(&error_diagnostics.diagnostics)
})
.header(
"Error detecting dependencies"
Expand All @@ -122,7 +116,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
"})
.print(),
RubyBuildpackError::RubyInstallError(error) => Announcement::error()
.detail(attn::Detail::Raw(error.to_string()))
.raw(&error)
.header(
"Error installing Ruby",
).body(formatdoc! {"
Expand All @@ -135,13 +129,11 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
})
.print(),
RubyBuildpackError::MissingGemfileLock(io_error_diagnostics) => Announcement::error()
.detail({
attn::Detail::Raw(io_error_diagnostics.error.to_string())
.raw({
&io_error_diagnostics.error
})
.detail({
attn::Detail::Raw(
build_output::fmt::cmd_diagnostics(&io_error_diagnostics.diagnostics)
)
.raw({
&build_output::fmt::cmd_diagnostics(&io_error_diagnostics.diagnostics)
})
.header(
"Gemfile.lock` not found"
Expand All @@ -158,7 +150,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
))
.print(),
RubyBuildpackError::RakeAssetsCacheError(error) => Announcement::error()
.detail(attn::Detail::Raw(error.to_string()))
.raw(&error)
.header(
"Error caching frontend assets"
)
Expand All @@ -168,7 +160,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
.body(file_hints)
.print(),
RubyBuildpackError::BundleInstallDigestError(error) => Announcement::error()
.detail(attn::Detail::Raw(error.to_string()))
.raw(&error.to_string())
.header(
"Failed to generate file digest"
)
Expand All @@ -184,9 +176,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
"})
.print(),
RubyBuildpackError::BundleInstallCommandError(error) => Announcement::error()
.detail({
attn::Detail::Raw(build_output::fmt::cmd_error(&error))
})
.raw(&build_output::fmt::cmd_error(&error))
.header(
"Failed to install bundler"
)
Expand All @@ -199,9 +189,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
let cmd_debug = local_command_debug(&error);

Announcement::error()
.detail({
attn::Detail::Raw(build_output::fmt::cmd_error(&error))
})
.raw(&build_output::fmt::cmd_error(&error))
.header("Failed to compile assets")
.body(formatdoc! {"
An error occured while compiling assets via rake command. Details of the error are
Expand All @@ -215,9 +203,7 @@ pub fn log_ruby_error(error: RubyBuildpackError) {
let cmd_debug = local_command_debug(&error);

Announcement::error()
.detail({
attn::Detail::Raw(build_output::fmt::cmd_error(&error))
})
.raw(&build_output::fmt::cmd_error(&error))
.header("Failed to install gems")
.body(formatdoc! {"
Could not install gems to the system via bundler. Gems are dependencies
Expand Down
36 changes: 11 additions & 25 deletions commons/src/build_output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -877,19 +877,6 @@ pub mod attn {
}
}

#[derive(Debug, PartialEq, Clone)]
pub enum Detail {
Raw(String),
}

impl Display for Detail {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Detail::Raw(details) => write!(f, "{details}"),
}
}
}

#[derive(Debug, PartialEq, Clone)]
pub enum Body {
Raw(String),
Expand All @@ -907,15 +894,15 @@ pub mod attn {
pub enum Part {
Body(Body),
Url(Url),
Detail(Detail),
Raw(String),
}

impl Display for Part {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Part::Body(body) => write!(f, "{body}"),
Part::Url(url) => write!(f, "{url}"),
Part::Detail(details) => write!(f, "{details}"),
Part::Raw(details) => write!(f, "{details}"),
}
}
}
Expand All @@ -939,7 +926,7 @@ pub mod attn {
// If both last and next lines share a prefix then add a prefix
// to the newline separator
let sep = match (now, next) {
(Part::Detail(_), _) | (_, Part::Detail(_)) => "\n".to_string(),
(Part::Raw(_), _) | (_, Part::Raw(_)) => "\n".to_string(),
_ => colorize(&self.color, bangify("\n")),
};

Expand All @@ -960,7 +947,7 @@ pub mod attn {
let part = match part {
Part::Body(body) => colorize(&self.color, bangify(body.to_string().trim())),
Part::Url(url) => colorize(&self.color, bangify(url.to_string().trim())),
Part::Detail(details) => details.to_string().trim().to_string(),
Part::Raw(details) => details.to_string().trim().to_string(),
};
format!("{part}\n")
}
Expand Down Expand Up @@ -1011,8 +998,8 @@ pub mod attn {

/// I don't love having this in here It's technically not part of the announcement
/// from a philosophical standpoint, however it makes a nice chained API
pub fn detail(&mut self, detail: Detail) -> &mut Self {
self.inner.push(Part::Detail(detail));
pub fn raw(&mut self, raw: &impl ToString) -> &mut Self {
self.inner.push(Part::Raw(raw.to_string()));
self
}

Expand All @@ -1025,7 +1012,6 @@ pub mod attn {
#[cfg(test)]
mod test {
use super::*;
use crate::build_output::attn;
use crate::build_output::fmt::strip_control_codes;
use crate::fun_run::{self, CmdMapExt, NamedOutput, ResultNameExt};
use indoc::formatdoc;
Expand All @@ -1043,10 +1029,10 @@ pub mod attn {
.url(Url::MoreInfo(
"https://devcenter.heroku.com/articles/ruby-support#ruby-versions".to_string(),
))
.detail(
Detail::Raw(formatdoc!{"
.raw(
&formatdoc!{"
Could not create file: You do not have sufficient permissions to access this file: /path/to/file
"})
"}
)
.to_string();

Expand Down Expand Up @@ -1078,7 +1064,7 @@ pub mod attn {
match result {
Ok(out) => panic!("Command should have failed {out:?}"),
Err(error) => {
let error_detail = attn::Detail::Raw(fmt::cmd_error(&error));
let error_detail = fmt::cmd_error(&error);
let expected = formatdoc! {"
- ! Command failed `cat does_not_exist` (details below)
- exit status: `1`
Expand All @@ -1095,7 +1081,7 @@ pub mod attn {
let actual = Announcement::error()
.header("Failed to compile assets")
.body("oops")
.detail(error_detail)
.raw(&error_detail)
.to_string();

let expected = formatdoc! {"
Expand Down

0 comments on commit 6cbe132

Please sign in to comment.