Fix warning/important/error formatting using trait

As described in the commit that introduced the failing test, this commit implements the "Logging state" trait approach.

To emit a warning/error/important to the output we must first `announce()`.

As I'm typing this up I realize that there's the case where you can `announce().end_announce()` we could patch this up by introducing an intermediate state that only has error, warn, important on it.

I don't like the duplicate code and duplicate trait definition, but the end result seems like a good API.

buildpacks/ruby/src/user_errors.rs

@@ -11,10 +11,10 @@ use commons::fun_run::{CmdError, CommandWithName};
 use indoc::formatdoc;
 
 pub(crate) fn on_error(err: libcnb::Error<RubyBuildpackError>) {
- let mut log = BuildLog::new(std::io::stdout()).without_buildpack_name();
+ let log = BuildLog::new(std::io::stdout()).without_buildpack_name();
  match cause(err) {
  Cause::OurError(error) => log_our_error(log, error),
- Cause::FrameworkError(_error) => log.error(&formatdoc! {"
+ Cause::FrameworkError(_error) => log.announce().error(&formatdoc! {"
  Error: heroku/buildpack-ruby internal buildpack error
 
  The framework used by this buildpack encountered an unexpected error.
@@ -59,7 +59,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  );
  }
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error: `Gemfile.lock` not found
 
  A `Gemfile.lock` file is required and was not found in the root of your application.
@@ -77,6 +77,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  // - In the future add a "did you mean" Levenshtein distance to see if they typoed like "3.6.0" when they meant "3.0.6"
  log.section(DEBUG_INFO)
  .step(&error.to_string())
+ .announce()
  .error(&formatdoc! {"
  Error installing Ruby
 
@@ -95,7 +96,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
 
  log = debug_cmd(log.section(DEBUG_INFO), Command::new("gem").arg("env"));
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error installing bundler
 
  The ruby package managment tool, `bundler`, failed to install. Bundler is required
@@ -115,6 +116,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  .section(DEBUG_INFO)
  .step(&error.to_string())
  .end_section()
+ .announce()
  .error(&formatdoc! {"
  Error installing your applications's dependencies
 
@@ -146,7 +148,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  );
  }
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error generating file digest
 
  An error occurred while generating a file digest. To provide the fastest possible
@@ -171,7 +173,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  .step(&error.to_string())
  .end_section();
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error detecting rake tasks
 
  The Ruby buildpack uses rake task information from your application to guide
@@ -189,7 +191,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  .step(&error.to_string())
  .end_section();
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error compiling assets
 
  An error occured while compiling assets via rake command.
@@ -208,7 +210,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
  .step(&error.to_string())
  .end_section();
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error caching frontend assets
 
  An error occurred while attempting to cache frontend assets, and the Ruby buildpack
@@ -228,7 +230,7 @@ fn log_our_error(mut log: Box<dyn StartedLogger>, error: RubyBuildpackError) {
 
  log = debug_cmd(log.section(DEBUG_INFO), Command::new("bundle").arg("env"));
 
- log.error(&formatdoc! {"
+ log.announce().error(&formatdoc! {"
  Error detecting dependencies
 
  The Ruby buildpack requires information about your application’s dependencies to

commons/bin/print_style_guide.rs

@@ -121,33 +121,33 @@ fn main() {
  .step(&cmd_error.to_string())
  .end_section();
 
- log.error(&formatdoc! {"
- Error: This is an error header
+ log.announce()
+ .warning(&formatdoc! {"
+ Warning: This is a warning header
 
- This is the error body. Use an error for when the build cannot continue.
- An error should include a header with a short description of why it cannot continue.
-
- The body should include what error state was observed, why that's a problem, and
- what remediation steps an application owner using the buildpack to deploy can
- take to solve the issue.
- "});
+ This is a warning body. Warnings are for when we know for a fact a problem exists
+ but it's not bad enough to abort the build.
+ "})
+ .important(&formatdoc! {"
+ Important: This is important
 
- log.warning(&formatdoc! {"
- Warning: This is a warning header
+ Important is for when there's critical information that needs to be read
+ however it may or may not be a problem. If we know for a fact that there's
+ a problem then use a warning instead.
 
- Warnings are for when we know for a fact a problem exists
- but it's not bad enough to abort the build.
- "});
- log.important(&formatdoc! {"
- Important: This is important
+  An example of something that is important but might not be a problem is
+  that an application owner upgraded to a new stack.
+  "})
+  .error(&formatdoc! {"
+  Error: This is an error header
 
- Important is for when there's critical information that needs to be read
- however it may or may not be a problem. If we know for a fact that there's
- a problem then use a warning instead.
+ This is the error body. Use an error for when the build cannot continue.
+ An error should include a header with a short description of why it cannot continue.
 
- An example of something that is important but might not be a problem is
- that an application owner upgraded to a new stack.
- "});
+ The body should include what error state was observed, why that's a problem, and
+ what remediation steps an application owner using the buildpack to deploy can
+ take to solve the issue.
+ "});
  }
 
  {

commons/src/output/build_log.rs

@@ -122,6 +122,16 @@ where
 
  writeln_now(&mut self.io, fmt::section(format!("Done {details}")));
  }
+
+ fn announce(mut self: Box<Self>) -> Box<dyn StartedAnnounceLogger> {
+ writeln_now(&mut self.io, "");
+
+ Box::new(AnnounceLogger {
+ io: self.io,
+ state: PhantomData::<state::Started>,
+ build_started: self.started,
+ })
+ }
 }
 impl<W> SectionLogger for BuildLog<state::InSection, W>
 where
@@ -179,29 +189,90 @@ where
  started: self.started,
  })
  }
+
+ fn announce(mut self: Box<Self>) -> Box<dyn SectionAnnounceLogger> {
+ writeln_now(&mut self.io, "");
+
+ Box::new(AnnounceLogger {
+ io: self.io,
+ state: PhantomData::<state::InSection>,
+ build_started: self.started,
+ })
+ }
 }
 
-impl<T, W> ErrorWarningImportantLogger for BuildLog<T, W>
+#[derive(Debug)]
+struct AnnounceLogger<T, W>
+where
+ W: Write + Send + Sync + Debug + 'static,
+{
+ pub(crate) io: W,
+ pub(crate) state: PhantomData<T>,
+ pub(crate) build_started: Instant,
+}
+
+impl<T, W> ErrorLogger for AnnounceLogger<T, W>
 where
  T: Debug,
- W: Write + Debug,
+ W: Write + Send + Sync + Debug + 'static,
 {
- fn warning(&mut self, s: &str) {
- writeln_now(&mut self.io, fmt::warn(s));
+ fn error(mut self: Box<Self>, s: &str) {
+ writeln_now(&mut self.io, fmt::error(s.trim()));
+ writeln_now(&mut self.io, "");
  }
+}
+
+impl<W> SectionAnnounceLogger for AnnounceLogger<state::InSection, W>
+where
+ W: Write + Send + Sync + Debug + 'static,
+{
+ fn warning(mut self: Box<Self>, s: &str) -> Box<dyn SectionAnnounceLogger> {
+ writeln_now(&mut self.io, fmt::warning(s.trim()));
+ writeln_now(&mut self.io, "");
 
- fn important(&mut self, s: &str) {
- writeln_now(&mut self.io, fmt::important(s));
+ self
+ }
+
+ fn important(mut self: Box<Self>, s: &str) -> Box<dyn SectionAnnounceLogger> {
+ writeln_now(&mut self.io, fmt::important(s.trim()));
+ writeln_now(&mut self.io, "");
+
+ self
+ }
+
+ fn end_announce(self: Box<Self>) -> Box<dyn SectionLogger> {
+ Box::new(BuildLog {
+ io: self.io,
+ state: PhantomData::<state::InSection>,
+ started: self.build_started,
+ })
  }
 }
 
-impl<T, W> ErrorLogger for BuildLog<T, W>
+impl<W> StartedAnnounceLogger for AnnounceLogger<state::Started, W>
 where
- T: Debug,
- W: Write + Debug,
+ W: Write + Send + Sync + Debug + 'static,
 {
- fn error(&mut self, s: &str) {
- writeln_now(&mut self.io, fmt::error(s));
+ fn warning(mut self: Box<Self>, s: &str) -> Box<dyn StartedAnnounceLogger> {
+ writeln_now(&mut self.io, fmt::warning(s.trim()));
+ writeln_now(&mut self.io, "");
+
+ self
+ }
+
+ fn important(mut self: Box<Self>, s: &str) -> Box<dyn StartedAnnounceLogger> {
+ writeln_now(&mut self.io, fmt::important(s.trim()));
+ writeln_now(&mut self.io, "");
+
+ self
+ }
+
+ fn end_announce(self: Box<Self>) -> Box<dyn StartedLogger> {
+ Box::new(BuildLog {
+ io: self.io,
+ state: PhantomData::<state::Started>,
+ started: self.build_started,
+ })
  }
 }
 
@@ -439,14 +510,13 @@ mod test {
  let writer = ReadYourWrite::writer(Vec::new());
  let reader = writer.reader();
 
- let mut logger = BuildLog::new(writer)
+ BuildLog::new(writer)
  .buildpack_name("RCT")
  .section("Guest thoughs")
- .step("The scenery here is wonderful");
-
- logger.warning("It's too crowded here\nI'm tired");
-
- logger
+ .step("The scenery here is wonderful")
+ .announce()
+ .warning("It's too crowded here\nI'm tired")
+ .end_announce()
  .step("The jumping fountains are great")
  .step("The music is nice here")
  .end_section()
@@ -476,15 +546,16 @@ mod test {
  let writer = ReadYourWrite::writer(Vec::new());
  let reader = writer.reader();
 
- let mut logger = BuildLog::new(writer)
+ let logger = BuildLog::new(writer)
  .buildpack_name("RCT")
  .section("Guest thoughs")
- .step("The scenery here is wonderful");
-
- logger.warning("It's too crowded here");
- logger.warning("I'm tired");
+ .step("The scenery here is wonderful")
+ .announce();
 
  logger
+ .warning("It's too crowded here")
+ .warning("I'm tired")
+ .end_announce()
  .step("The jumping fountains are great")
  .step("The music is nice here")
  .end_section()

commons/src/output/fmt.rs

@@ -141,7 +141,7 @@ pub(crate) fn important(contents: impl AsRef<str>) -> String {
 }
 
 #[must_use]
-pub(crate) fn warn(contents: impl AsRef<str>) -> String {
+pub(crate) fn warning(contents: impl AsRef<str>) -> String {
  colorize(WARNING_COLOR, bangify(contents))
 }
 

commons/src/output/interface.rs

@@ -6,35 +6,46 @@ pub trait Logger: Debug {
  fn without_buildpack_name(self) -> Box<dyn StartedLogger>;
 }
 
-pub trait StartedLogger: ErrorWarningImportantLogger + Debug {
+pub trait StartedLogger: Debug {
  fn section(self: Box<Self>, s: &str) -> Box<dyn SectionLogger>;
  fn finish_logging(self: Box<Self>);
-}
 
-pub trait StreamLogger: Debug {
- fn io(&mut self) -> Box<dyn Write + Send + Sync + 'static>;
- fn finish_timed_stream(self: Box<Self>) -> Box<dyn SectionLogger>;
+ fn announce(self: Box<Self>) -> Box<dyn StartedAnnounceLogger>;
 }
 
-pub trait SectionLogger: ErrorWarningImportantLogger + Debug {
+pub trait SectionLogger: Debug {
  fn step(self: Box<Self>, s: &str) -> Box<dyn SectionLogger>;
  fn mut_step(&mut self, s: &str);
  fn step_timed(self: Box<Self>, s: &str) -> Box<dyn TimedStepLogger>;
  fn step_timed_stream(self: Box<Self>, s: &str) -> Box<dyn StreamLogger>;
  fn end_section(self: Box<Self>) -> Box<dyn StartedLogger>;
+
+ fn announce(self: Box<Self>) -> Box<dyn SectionAnnounceLogger>;
+}
+
+pub trait StartedAnnounceLogger: ErrorLogger + Debug {
+ fn warning(self: Box<Self>, s: &str) -> Box<dyn StartedAnnounceLogger>;
+ fn important(self: Box<Self>, s: &str) -> Box<dyn StartedAnnounceLogger>;
+
+ fn end_announce(self: Box<Self>) -> Box<dyn StartedLogger>;
+}
+
+pub trait SectionAnnounceLogger: ErrorLogger + Debug {
+ fn warning(self: Box<Self>, s: &str) -> Box<dyn SectionAnnounceLogger>;
+ fn important(self: Box<Self>, s: &str) -> Box<dyn SectionAnnounceLogger>;
+
+ fn end_announce(self: Box<Self>) -> Box<dyn SectionLogger>;
 }
 
 pub trait TimedStepLogger: Debug {
  fn finish_timed_step(self: Box<Self>) -> Box<dyn SectionLogger>;
 }
 
-// Object safety needs to be sorted out
-pub trait ErrorWarningImportantLogger: ErrorLogger + Debug {
- /// TODO: make this chainable
- fn warning(&mut self, s: &str);
- fn important(&mut self, s: &str);
+pub trait StreamLogger: Debug {
+ fn io(&mut self) -> Box<dyn Write + Send + Sync + 'static>;
+ fn finish_timed_stream(self: Box<Self>) -> Box<dyn SectionLogger>;
 }
 
 pub trait ErrorLogger: Debug {
- fn error(&mut self, s: &str);
+ fn error(self: Box<Self>, s: &str);
 }

commons/src/output/section_log.rs

@@ -101,17 +101,17 @@ pub fn log_step_stream<T>(
 
 /// Print an error block to the output
 pub fn log_error(s: impl AsRef<str>) {
- logger().error(s.as_ref());
+ logger().announce().error(s.as_ref());
 }
 
 /// Print an warning block to the output
 pub fn log_warning(s: impl AsRef<str>) {
- logger().warning(s.as_ref());
+ logger().announce().warning(s.as_ref());
 }
 
 /// Print an important block to the output
 pub fn log_important(s: impl AsRef<str>) {
- logger().important(s.as_ref());
+ logger().announce().important(s.as_ref());
 }
 
 fn logger() -> Box<dyn SectionLogger> {