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

Clarify documentation and simplify initialization of STATIC_MAX_LEVEL #594

Merged
merged 4 commits into from
Nov 19, 2023

Conversation

ptosi
Copy link
Contributor

@ptosi ptosi commented Oct 20, 2023

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.

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.
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Doc changes LGTM.

LevelFilter::Trace
}
}
pub const STATIC_MAX_LEVEL: LevelFilter = match cfg!(debug_assertions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will work with our MSRV?

Copy link
Contributor Author

@ptosi ptosi Oct 20, 2023

Choose a reason for hiding this comment

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

Interesting, which bit?

I've checked these changes with cargo +1.60.0 test; how do you recommend I test it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, log used to have a MSRV of 1.34 or something, but 1.60 should work 👍

LevelFilter::Trace
}
}
pub const STATIC_MAX_LEVEL: LevelFilter = match cfg!(debug_assertions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, log used to have a MSRV of 1.34 or something, but 1.60 should work 👍

@KodrAus KodrAus merged commit b7c4f6f into rust-lang:master Nov 19, 2023
14 checks passed
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