Implement Serialization and Deserialization for Verbosity #114

joshka · 2024-09-20T11:39:38Z

Verbosity is serialized and deserialized using the title case of the
VerbosityFilter (e.g. "Debug")

The serde dependency is gated behind an optional feature flag.

Added conversion methods between Verbosity and VerbosityFilter to
simplify the implementation and derived PartialEq, Eq impls on
types where this was necesary for testing.

Fixes: #88

src/lib.rs

src/serde.rs

src/lib.rs

coveralls · 2024-09-20T14:17:58Z

Pull Request Test Coverage Report for Build 12205777991

Details

8 of 12 (66.67%) changed or added relevant lines in 1 file are covered.
8 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 49.541%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib.rs	8	12	66.67%

Files with Coverage Reduction	New Missed Lines	%
src/lib.rs	8	49.28%

Totals
Change from base Build 12101205082:	-0.5%
Covered Lines:	54
Relevant Lines:	109

💛 - Coveralls

src/serde.rs

Cargo.toml

src/serde.rs

joshka

I will:

remove rstest
serialize to/from LevelFilter instead of Option(Level)
add a specific test for TOML serialization
Fix the github lints
split the i8 change out to another commit (and will raise another PR for it if necessary)

Cargo.toml

src/lib.rs

src/serde.rs

Cargo.toml

joshka

Moving to LevelFilter made it easy to derive serialize / deserialize using From<LevelFilter> instead of a custom implementation.

Cargo.toml

src/lib.rs

src/serde.rs

src/lib.rs

epage · 2024-09-26T01:54:09Z

src/lib.rs

+        }
+
+        // round-trips
+        let toml = "meaning_of_life = 42\nverbose = \"DEBUG\"\n";


Not sure how I feel about deserializing to all-caps

Yeah, this irks me a little too, but being consistent with the existing behavior won out over my ideal approach. If I was designing the log crate, I think I'd have made that serialization lower case too.

Here, this can deserialize from either case, but serializes to upper case. It comes from the serialization of log::LevelFilter. That means upper case is consistent with any other serialization that works with log level serialization (and is consistent with the usual display of the level in log outputs).

If you did want to avoid this, it would require going back to using a custom serialization implementation. This doesn't seem worth the extra code and tests to me. I prefer the simplicity even if that comes with a slightly ugly default which is consistent with other things doing the same thing.

I also suspect the serialization side of this will be used much less frequently than the deserialization side, and the serialization side will also often be more focused on programs re-reading the format rather than humans.

On one hand, if people are serializing LevelFilter, this keeps us consistent with that. On the other hand, this will produce results inconsistent with every other serialized value. This isn't just important here but for other cases like schemars. I think I want to let this sit for a time to mull over this.

no problem :)

eb58de2 serializes to VerbosityFilter instead of this, which doesn't have a deserialization implementation that handles upper case tokens.

That replaced SCREAMING CASE with Title case

assert_eq!(toml, "meaning_of_life = 42\nverbosity = \"Debug\"\n");

I feel like lower case is much more likely to align with a users configuration format design (as most are likely kebab-case or snake_case)

btw eb58de2 just changed the value but it doesn't answer the question about interoperability

src/lib.rs

joshka · 2024-11-29T01:27:36Z

In eb58de2, I rewrote this based on the 3.0 release. Notably this uses the VerbosityFilter to de/serialize instead of log::LogLevelFilter which means that it only handles Title case deserialization. I've also simplified the style and quantity of tests significantly.

Verbosity is serialized and deserialized using the title case of the VerbosityFilter (e.g. "Debug") The `serde` dependency is gated behind an optional feature flag. Added conversion methods between Verbosity and VerbosityFilter to simplify the implementation and derived PartialEq, Eq impls on types where this was necesary for testing. Fixes: clap-rs#88

epage · 2024-12-06T20:39:42Z

src/lib.rs

@@ -70,8 +70,21 @@ pub mod log;
 pub mod tracing;

 /// Logging flags to `#[command(flatten)]` into your CLI
-#[derive(clap::Args, Debug, Clone, Default)]
+#[derive(clap::Args, Debug, Clone, Default, PartialEq, Eq)]


This PR seems to be mixing a lot of different things

Making types support Eq

More From impls

A refactor with how the count math is done

etc

Ideally these would at least be split into individual commits

We can track them in release notes more easily

I can better see how all of the parts for a single change fit together

Cargo.toml