Skip to content

Conversation

@tlater-famedly
Copy link
Contributor

@tlater-famedly tlater-famedly commented May 6, 2025

A half-applied suggestion from review during the release PR in #31 broke features that depend on serde. Since we have to fix this anyway, might as well go all the way and set features to depend on each other, and boot serde wherever possible.

A half-applied suggestion from review during the release PR in #31
broke features that depend on serde.
@tlater-famedly tlater-famedly requested a review from a team as a code owner May 6, 2025 07:31
@codecov
Copy link

codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.02%. Comparing base (224983e) to head (1757283).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #32   +/-   ##
=======================================
  Coverage   64.02%   64.02%           
=======================================
  Files           6        6           
  Lines         378      378           
=======================================
  Hits          242      242           
  Misses        136      136           
Files with missing lines Coverage Δ
src/level_filter.rs 57.62% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 224983e...1757283. Read the comment docs.

config = ["dep:figment", "dep:serde"]
level_filter = ["dep:tracing", "dep:serde"]
config = ["dep:figment", "serde"]
level_filter = ["dep:tracing"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LevelFilter is useless without serde, featuregating whole crate::level_filter with serde seems like a good idea. Can you clarify what is broken? Maybe an example of a feature set that fails to compile?

Copy link
Contributor Author

@tlater-famedly tlater-famedly Aug 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been 3 months :D I think the FromStr and Fmt impls were still useful, at least in tests somewhere. I'll see if I can dig up the reasoning again, but I'm unsure I'll find it. I did consider this, I swear, not including serde was a conscious decision - wish I'd left a comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants