rust-cli / env_logger

A logging implementation for `log` which is configured via an environment variable.
https://docs.rs/env_logger
Apache License 2.0
803 stars 125 forks source link

add unit tests, docs related to available log level names #189

Closed salewski closed 3 years ago

salewski commented 3 years ago

This PR adds some unit tests and freshens up docs related to logging levels.

This work started as an effort to provide unit tests that exercise simple scenarios in which a single logging level is specified for the entire application (without any more specific logging directives). It grew into a bit of a doc updating effort, too.

The unifying theme behind the individual commits in this series is the surfacing of simple facts about the env_logger crate. These changes help provide answers to some fairly basic questions such as:

The doc changes also include fixes to some contradictory statements about what the default behavior is.

Most of this is just gathering data from elsewhere else and putting it where people are already looking (info from the README.md file that wasn't in the crate-level API docs (or vice versa); info that can be guessed at from glancing at the log crate's API docs, but which are important for using env_logger with confidence; and so on). The idea is that the user reading the crate-level API docs on crates.io should get roughly the same sense for the crate as does the person reading the project's 'README.md' file on GitHub.

The newly added unit tests exercise functionality in a way that is more basic than the existing tests (that involve more sophisticated logging directives). These new tests also serve as easy to locate in-tree examples of simple use.

All in all, these changes help make the documentation of env_logger more self contained -- able to stand better on its own, and with less reliance on the user's familiarity with the log crate.

salewski commented 3 years ago

Heads-up: The failed CI check is unrelated to this PR.

The "Check formatting" check is complaining about a trailing comma on an existing line of code, not something introduced here.

salewski commented 3 years ago

@jplatte Thanks for the review!

Thanks for the PR, I like the improved docs!

I've now fixed the rustfmt issue on the master branch, could you rebase?

Sure; I'll do that as soon as I make the adjustments outlined below.

Also, why do your commit messages all start with [N of 9]:? I've seen this before but don't really understand what value it provides.

It's mainly a way of organizing work informed by experience reviewing patch series in email threads. It attaches to a commit useful metadata that will live with the commit indefinitely (unless it gets removed in a future rebase or "squash and merge"). Having that small bit of metadata handy avoids the need to re-derive it, and provides a number of small benefits that can make things easier to manage, especially when working with a large number of projects and branches within those projects.

salewski commented 3 years ago

@jplatte By the way, thanks for fixing the rustfmt issue!

jplatte commented 3 years ago

@jplatte By the way, thanks for fixing the rustfmt issue!

I only re-ran cargo fmt :sweat_smile: (although I would really prefer the formatting as it was too)

Also, why do your commit messages all start with [N of 9]:? I've seen this before but don't really understand what value it provides.

[...]

I was not expecting such an elaborate reply! Doing a lot of rebases myself I don't really get the benefits for that, but I can see how it would be nice to see when reviewing large change sets as a quick gauge for how far into the review one is.

I'll be doing the default GitHub "merge", i.e. with a merge commit, if that matters.

salewski commented 3 years ago

I was not expecting such an elaborate reply!

Yeah, I kinda got on roll there... :-)

I'll be doing the default GitHub "merge", i.e. with a merge commit, if that matters.

I generally prefer that , too, because it keeps a more perfect record of the already-massaged history.

I just pushed my rebased update. It's identical to the previous set with two exceptions:

Please take a look when you can. I'm happy to refine it further, too.