draft: try to make Pretty's field formatter respect the writer's ansi setting

[davidbarsky]

this isn't done yet, as one of the field formatters doesn't respect the writer's ansi settings. still digging into the cause.

[hawkw]

I think this change is actually much easier than you think it is. It looks like currently, you've modified the code in the visitor so that there's a separate path that is entered when ANSI escapes are not enabled. But, the self.bold() call already returns a no-op Style when ANSI escapes are disabled, so this isn't actually necessary. Instead, the thing we need to change is that when the PrettyFields type is constructed by PrettyFields::new(), it defaults to enabling ANSI escapes, and when that field formatter constructs a PrettyVisitor, it passes on that configuration to the PrettyVisitor, overriding the configuration on the Writer:

tracing/tracing-subscriber/src/fmt/format/pretty.rs

Lines 241 to 270 in c90d22b

	impl Default for PrettyFields {
	fn default() -> Self {
	Self::new()
	}
	}
	impl PrettyFields {
	/// Returns a new default [`PrettyFields`] implementation.
	pub fn new() -> Self {
	Self { ansi: true }
	}
	#[deprecated(
	since = "0.3.3",
	note = "Use `fmt::Subscriber::with_ansi` or `fmt::Collector::with_ansi` instead."
	)]
	/// Enable ANSI encoding for formatted fields.
	pub fn with_ansi(self, ansi: bool) -> Self {
	Self { ansi, ..self }
	}
	}
	impl<'a> MakeVisitor<Writer<'a>> for PrettyFields {
	type Visitor = PrettyVisitor<'a>;
	#[inline]
	fn make_visitor(&self, target: Writer<'a>) -> Self::Visitor {
	PrettyVisitor::new(target.with_ansi(self.ansi), true)
	}
	}

I think what we want to do here is just to change PrettyFields to not always clobber the writer's ANSI setting, and instead only do it when the user asks it to. This patch should fix that:

diff --git a/tracing-subscriber/src/fmt/format/pretty.rs b/tracing-subscriber/src/fmt/format/pretty.rs
index 542dc116..9d926ab1 100644
--- a/tracing-subscriber/src/fmt/format/pretty.rs
+++ b/tracing-subscriber/src/fmt/format/pretty.rs
@@ -39,7 +39,17 @@ pub struct PrettyVisitor<'a> {
 /// [`MakeVisitor`]: crate::field::MakeVisitor
 #[derive(Debug)]
 pub struct PrettyFields {
-    ansi: bool,
+    /// A value to override the provided `Writer`'s ANSI formatting
+    /// configuration.
+    ///
+    /// If this is `Some`, we override the `Writer`'s ANSI setting. This is
+    /// necessary in order to continue supporting the deprecated
+    /// `PrettyFields::with_ansi` method. If it is `None`, we don't override the
+    /// ANSI formatting configuration (because the deprecated method was not
+    /// called).
+    // TODO: when `PrettyFields::with_ansi` is removed, we can get rid
+    // of this entirely.
+    ansi: Option<bool>,
 }
 
 // === impl Pretty ===
@@ -247,7 +257,12 @@ impl Default for PrettyFields {
 impl PrettyFields {
     /// Returns a new default [`PrettyFields`] implementation.
     pub fn new() -> Self {
-        Self { ansi: true }
+        Self {
+            // By default, don't override the `Writer`'s ANSI colors
+            // configuration. We'll only do this if the user calls the
+            // deprecated `PrettyFields::with_ansi` method.
+            ansi: None,
+        }
     }
 
     #[deprecated(
@@ -256,7 +271,10 @@ impl PrettyFields {
     )]
     /// Enable ANSI encoding for formatted fields.
     pub fn with_ansi(self, ansi: bool) -> Self {
-        Self { ansi, ..self }
+        Self {
+            ansi: Some(ansi),
+            ..self
+        }
     }
 }
 
@@ -264,8 +282,11 @@ impl<'a> MakeVisitor<Writer<'a>> for PrettyFields {
     type Visitor = PrettyVisitor<'a>;
 
     #[inline]
-    fn make_visitor(&self, target: Writer<'a>) -> Self::Visitor {
-        PrettyVisitor::new(target.with_ansi(self.ansi), true)
+    fn make_visitor(&self, mut target: Writer<'a>) -> Self::Visitor {
+        if let Some(ansi) = self.ansi {
+            target = target.with_ansi(ansi);
+        }
+        PrettyVisitor::new(target, true)
     }
 }
 
@@ -347,42 +368,27 @@ impl<'a> field::Visit for PrettyVisitor<'a> {
         if self.result.is_err() {
             return;
         }
-        if self.writer.has_ansi_escapes() {
-            let bold = self.bold();
-            match field.name() {
-                "message" => {
-                    self.write_padded(&format_args!("{}{:?}", self.style.prefix(), value,))
-                }
-                // Skip fields that are actually log metadata that have already been handled
-                #[cfg(feature = "tracing-log")]
-                name if name.starts_with("log.") => self.result = Ok(()),
-                name if name.starts_with("r#") => self.write_padded(&format_args!(
-                    "{}{}{}: {:?}",
-                    bold.prefix(),
-                    &name[2..],
-                    bold.infix(self.style),
-                    value
-                )),
-                name => self.write_padded(&format_args!(
-                    "{}{}{}: {:?}",
-                    bold.prefix(),
-                    name,
-                    bold.infix(self.style),
-                    value
-                )),
-            };
-        } else {
-            match field.name() {
-                "message" => self.write_padded(&format_args!("{:?}", value)),
-                // Skip fields that are actually log metadata that have already been handled
-                #[cfg(feature = "tracing-log")]
-                name if name.starts_with("log.") => self.result = Ok(()),
-                name if name.starts_with("r#") => {
-                    self.write_padded(&format_args!("{}: {:?}", &name[2..], value))
-                }
-                name => self.write_padded(&format_args!("{}: {:?}", name, value)),
-            };
-        }
+        let bold = self.bold();
+        match field.name() {
+            "message" => self.write_padded(&format_args!("{}{:?}", self.style.prefix(), value,)),
+            // Skip fields that are actually log metadata that have already been handled
+            #[cfg(feature = "tracing-log")]
+            name if name.starts_with("log.") => self.result = Ok(()),
+            name if name.starts_with("r#") => self.write_padded(&format_args!(
+                "{}{}{}: {:?}",
+                bold.prefix(),
+                &name[2..],
+                bold.infix(self.style),
+                value
+            )),
+            name => self.write_padded(&format_args!(
+                "{}{}{}: {:?}",
+                bold.prefix(),
+                name,
+                bold.infix(self.style),
+                value
+            )),
+        };
     }
 }
 

[davidbarsky]

I think what we want to do here is just to change PrettyFields to not always clobber the writer's ANSI setting, and instead only do it when the user asks it to. This patch should fix that:

Whoops, thanks for your help here. I didn't realize it was that simple!

[hawkw]

looks good to me, thanks!