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

Make filtering public #29

Merged
merged 3 commits into from
Oct 30, 2017
Merged

Make filtering public #29

merged 3 commits into from
Oct 30, 2017

Conversation

KodrAus
Copy link
Collaborator

@KodrAus KodrAus commented Oct 23, 2017

This is an attempt to make env_loggers filtering more of a first-class API that people can use to build their own loggers.

I think @jethrogb's solution is actually super clever and required basically no extra code to make it work. I never would've thought to do that. But I feel like we could make filtering even more first-class at the expense of some more code to maintain. Having a Target::Silent seems a little odd, because you would never want to actually install an empty logger as the default, and is a bit of a roundabout way to get at the filtering.

There's still some cleanup and docs to do here, but I thought I'd open it up and see what people think. From the user's point of view the code looks basically the same, but I think the intent is a bit clearer when you're building your own logger, which is an important cop-out to supporting targets like files in env_logger.

r? @jethrogb

@jethrogb
Copy link
Contributor

This looks mostly fine to me. The Target::Silent was kind of useless anyway, just there in case you accidentally use the logger for real.

  1. Because my previous change was so minimally-invasive, I'm not super familiar with the code base.
  2. I think it's pretty uncommon to have a Builder that contains another Builder inside. Normally you'd have the user construct the inner Builder manually first, build it, and then pass the built filter to the outer Builder. Are you using the current construction just for backcompat?
  3. Agree docs need improvement.

@KodrAus
Copy link
Collaborator Author

KodrAus commented Oct 24, 2017

I think it's pretty uncommon to have a Builder that contains another Builder inside. Normally you'd have the user construct the inner Builder manually first, build it, and then pass the built filter to the outer Builder. Are you using the current construction just for backcompat?

That's right. env_logger has a lot of dependents so I don't want to shift a lot of cheese upfront. In the future we may want to work towards DRYing out the duplicated builder methods.

@KodrAus KodrAus changed the title [WIP] Make filtering public Make filtering public Oct 29, 2017
@KodrAus KodrAus merged commit 955b1bd into master Oct 30, 2017
@KodrAus KodrAus deleted the feat/public-filter branch October 31, 2017 22:12
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.

2 participants