Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow constructing LevelFilter via incrementation #624

Open
neithernut opened this issue Mar 29, 2024 · 9 comments
Open

Allow constructing LevelFilter via incrementation #624

neithernut opened this issue Mar 29, 2024 · 9 comments

Comments

@neithernut
Copy link

When implementing a command line application where logging is somewhat important (e.g. a longer-running service), I sometimes like to make the verbosity configurable via multiple sources such as configuration files, environment variables and command line options.
However, for the latter I usually want a somewhat different behavior. Rather than the number of -v (or --verbose) flags setting the verbosity directly, I want each -v to increase the level from whatever the baseline is, which may come from a config file or some default level.

Currently, this requires some awkward code in applications. If we had the possibility to create a LevelFilter from an integer, this would be trivial. This was proposed in the past but clearly rejected (#318, #460). However, I didn't yet find anyone proposing the possibility of creating a level by incrementing some LevelFilter.

That could be done by either:

  • adding a new fn increment(self)/fn (self) to LevelFilter,
  • a new fn iter_from(self) to LevelFilter (whcih would be identical to the existing iter(self) and save users at least some awkward filtering),
  • implementing std::ops::Add<usize> and/or std::ops::AddAssign<usize> for LevelFilter or
  • implementing the currently unstable std::iter::Step for LevelFilter.

The latter clearly needed to be feature gated (which would be awkward) and/or increase the MSRV, but would allow using LevelFilter with std::ops::Ranges as an Iterator in the future. And of course, those options are not mutually exclusive. The std::iter::Step impl can follow at some later point if/when it gets stabilized.

@Thomasdezeeuw
Copy link
Collaborator

You can use min((level_filter as usize) + 1, Trace::Trace as usize) as LevelFilter as LevelFilter has #[repr(usize)] to do this already.

I'm not sure this a common enough case that we need the crate, though.

@neithernut
Copy link
Author

Are you sure? All versions of Rust which I tried this on say that the cast from usize to LevelFilter is illegal1. Given that the representation is known you can still hack the equivalent of C++'s reinterpret_cast, but...

Footnotes

  1. I would provide a link to https://play.rust-lang.org, but it appears to be broken right now.

@Thomasdezeeuw
Copy link
Collaborator

This works: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1f583a914a1b153a80213f3cd78c8676 and it's safe only because of #[repr(usize)]. However you have to make sure the value is valid, i.e. not bigger than 5/LevelFilter::Trace, but the transmute itself is not UB.

@neithernut
Copy link
Author

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

@Thomasdezeeuw
Copy link
Collaborator

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

Not really, the compiler can't ensure you do 6 as LevelFilter, which would be UB (5 is max valid value).

@neithernut
Copy link
Author

Agreed, but it would still be nice not having to resort to unsafe, even if it is factually safe.

Not really, the compiler can't ensure you do 6 as LevelFilter, which would be UB (5 is max valid value).

Yes, I know. I meant that with an API like TryFrom<usize> or the ones I suggested in my initial post would remove the need for library users to use unsafe code like the one you posted earlier.

@Thomasdezeeuw
Copy link
Collaborator

Which brings us back to

I'm not sure this a common enough case that we need the crate, though.

@Zoybean
Copy link

Zoybean commented Jun 2, 2024

I agree that some kind of increment/decrement on log levels and/or filters would be useful. It would also circumvent the stated reasons the other issues were rejected (#318, #460). Namely, it doesn't increase exposure of the underlying integer representation to users. It still does, however, expose the exact sequence of elements, e.g. adding a new level between two existing levels might break someone's code. I don't know if that is an issue.

@neithernut
Copy link
Author

It still does, however, expose the exact sequence of elements, e.g. adding a new level between two existing levels might break someone's code. I don't know if that is an issue.

The sequence is already exposed via the Iterator returned by LevelFilter::iter. A new feature for iterating over levels will thus at least not introduce this issue.

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

No branches or pull requests

3 participants