From 399e707a402386d64ebeb29ba12a5941a13eb3f9 Mon Sep 17 00:00:00 2001
From: Eliza Weisman <eliza@buoyant.io>
Date: Mon, 10 Oct 2022 12:10:58 -0700
Subject: [PATCH] subscriber: impl `fmt::Display` for `filter::Targets` (#2343)

## Motivation

There's currently a `fmt::Display` impl for `EnvFilter` that emits an
equiovalent filter string that can be parsed back into an `EnvFilter`,
but the `Targets` filter does not have a `fmt::Display` impl. We ought
to have one, especially to make using `Targets` with `clap` v4.0 easier.

## Solution

This branch adds a `fmt::Display` impl for `filter::Targets`. The
implementation is pretty straightforward.

I also added tests that a `Targets`' `fmt::Display` output can be parsed
back into a filter that's equivalent to the original.
---
 tracing-subscriber/src/filter/targets.rs | 53 ++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/tracing-subscriber/src/filter/targets.rs b/tracing-subscriber/src/filter/targets.rs
index 58ccfe4d21..19f2d99086 100644
--- a/tracing-subscriber/src/filter/targets.rs
+++ b/tracing-subscriber/src/filter/targets.rs
@@ -16,6 +16,7 @@ use crate::{
 #[cfg(not(feature = "std"))]
 use alloc::string::String;
 use core::{
+    fmt,
     iter::{Extend, FilterMap, FromIterator},
     slice,
     str::FromStr,
@@ -488,6 +489,20 @@ impl<'a> IntoIterator for &'a Targets {
     }
 }
 
+impl fmt::Display for Targets {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let mut directives = self.0.directives();
+        if let Some(directive) = directives.next() {
+            write!(f, "{}", directive)?;
+            for directive in directives {
+                write!(f, ",{}", directive)?;
+            }
+        }
+
+        Ok(())
+    }
+}
+
 /// An owning iterator over the [target]-[level] pairs of a `Targets` filter.
 ///
 /// This struct is created by the `IntoIterator` trait implementation of [`Targets`].
@@ -778,4 +793,42 @@ mod tests {
             crate2=debug,crate3=trace,crate3::mod2::mod1=off",
         );
     }
+
+    /// Test that the `fmt::Display` implementation for `Targets` emits a string
+    /// that can itself be parsed as a `Targets`, and that the parsed `Targets`
+    /// is equivalent to the original one.
+    #[test]
+    fn display_roundtrips() {
+        fn test_roundtrip(s: &str) {
+            let filter = expect_parse(s);
+            // we don't assert that the display output is equivalent to the
+            // original parsed filter string, because the `Display` impl always
+            // uses lowercase level names and doesn't use the
+            // target-without-level shorthand syntax. while they may not be
+            // textually equivalent, though, they should still *parse* to the
+            // same filter.
+            let formatted = filter.to_string();
+            let filter2 = match dbg!(&formatted).parse::<Targets>() {
+                Ok(filter) => filter,
+                Err(e) => panic!(
+                    "failed to parse formatted filter string {:?}: {}",
+                    formatted, e
+                ),
+            };
+            assert_eq!(filter, filter2);
+        }
+
+        test_roundtrip("crate1::mod1=error,crate1::mod2,crate2=debug,crate3=off");
+        test_roundtrip(
+            "crate1::mod1=ERROR,crate1::mod2=WARN,crate1::mod2::mod3=INFO,\
+        crate2=DEBUG,crate3=TRACE,crate3::mod2::mod1=OFF",
+        );
+        test_roundtrip(
+            "crate1::mod1=error,crate1::mod2=warn,crate1::mod2::mod3=info,\
+             crate2=debug,crate3=trace,crate3::mod2::mod1=off",
+        );
+        test_roundtrip("crate1::mod1,crate1::mod2,info");
+        test_roundtrip("crate1");
+        test_roundtrip("info");
+    }
 }