Clarify documentation and simplify initialization of STATIC_MAX_LEVEL
#594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rephrase the documentation to make it clear that the max_level_* and release_max_level_* are not separate ways of setting STATIC_MAX_LEVEL for respectively debug and release builds but instead, that max_level_* are used by all builds and that release_max_level_* are just a way to override them.
Add unit tests that check, for the current configuration of the crate
features, that STATIC_MAX_LEVEL has the appropriate value. As the
features are statically set (at build time), coverage of all the
max_level_* and release_max_level_* value pairs must be achieved by
launching cargo with different sets:
levels="off debug error info trace warn"
for i in '' $levels; do
release=${i:+--features=release_max_level_${i}}
for j in '' $levels; do
debug=${j:+--features=max_level_${j}}
cargo test static_max_level ${release} ${debug}
cargo test static_max_level ${release} ${debug} --release
done
done
Replace get_max_level_inner() with cfg!() and a const-compatible match, now that the MSRV has been bumped to 1.60.0.
Drop the constant as it now seems unnecessary. Note: this is done in a separate commit to be easy to bisect and revert.
| LevelFilter::Trace | ||
| } | ||
| } | ||
| pub const STATIC_MAX_LEVEL: LevelFilter = match cfg!(debug_assertions) { |
There was a problem hiding this comment.
I don't think this will work with our MSRV?
There was a problem hiding this comment.
Interesting, which bit?
I've checked these changes with cargo +1.60.0 test; how do you recommend I test it?
There was a problem hiding this comment.
Right, log used to have a MSRV of 1.34 or something, but 1.60 should work 👍
Thomasdezeeuw
approved these changes
Oct 20, 2023
| LevelFilter::Trace | ||
| } | ||
| } | ||
| pub const STATIC_MAX_LEVEL: LevelFilter = match cfg!(debug_assertions) { |
There was a problem hiding this comment.
Right, log used to have a MSRV of 1.34 or something, but 1.60 should work 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rephrase the documentation to make it clear that the
max_level_*features are also used by release builds.With the MSRV bump, new language features allow initializing the constant in a way that's conciser and hopefully clearer.
Add some tests, used to validate the change.