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

feat: Feature-flagged logging #29

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

Pscheidl
Copy link
Contributor

@Pscheidl Pscheidl commented Mar 8, 2023

Disabling logging in production via release_max_level_off disables logging for the whole resulting lib/binary, not just the serde-env crate. A workaround would be an opt-in feature, disabled by default.

@Pscheidl
Copy link
Contributor Author

Pscheidl commented Mar 8, 2023

Created a tiny project to prove this log flag actually does behave like this. https://github.com/Pscheidl/logtest

Libraries should avoid using the max level features because they’re global and can’t be changed once they’re set.

@Xuanwo
Copy link
Owner

Xuanwo commented Mar 8, 2023

Oh, that's a valid point. However, adding a feature flag for every log call can be quite tedious. Would it be possible to create an internal module called logging and provide different logging backends for it? For instance, we could use a dummy logging in the release build and use log in the debug build.

@Pscheidl
Copy link
Contributor Author

Pscheidl commented Mar 8, 2023

Oh, that's a valid point. However, adding a feature flag for every log call can be quite tedious. Would it be possible to create an internal module called logging and provide different logging backends for it? For instance, we could use a dummy logging in the release build and use log in the debug build.

It can be done (updated this MR), but it creates another problem. Unused variables on release. I don't know how to create a workaround for this, without duplicating macros from the log crate.

pavel@machine:~/Work/serde-env$ cargo build --release
   Compiling serde-env v0.1.1 (/home/pavel/Work/serde-env)
warning: unused variable: `name`
   --> src/de.rs:264:9
    |
264 |         name: &'static str,
    |         ^^^^ help: if this is intentional, prefix it with an underscore: `_name`
    |
    = note: `#[warn(unused_variables)]` on by default

warning: unused variable: `name`
   --> src/de.rs:292:9
    |
292 |         name: &'static str,
    |         ^^^^ help: if this is intentional, prefix it with an underscore: `_name`

warning: `serde-env` (lib) generated 2 warnings
    Finished release [optimized] target(s) in 0.35s

@Xuanwo
Copy link
Owner

Xuanwo commented Mar 8, 2023

Let's merge first, we can polish this part later.

@Xuanwo Xuanwo merged commit c309bca into Xuanwo:main Mar 8, 2023
@Pscheidl Pscheidl deleted the logging-feature-flagged branch March 8, 2023 11:39
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.

None yet

2 participants