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

Show target instead of module path by default #209

Merged
merged 4 commits into from Jul 14, 2021
Merged

Conversation

gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Jul 3, 2021

This is an alternative to #205, since that pr seems to be incomplete/stalled.

  • d2998a6 Added the option of printing the log target in the header
  • 365ffaf Changed defaults:
    • Module path is now hidden by default
    • Target is now shown by default
  • 1888497 Clarified the difference between module path and log target

Fixes #155
Fixes #179

These changes also likely warrant a minor version bump since they alter the default output format - but I'm leaving it out of the pr since I'm unsure of what the exact roadmap is, if any.

@jplatte
Copy link
Contributor

jplatte commented Jul 3, 2021

that pr seems to be incomplete/stalled

I think the main problem is there is nobody actively maintaining this crate. I'm helping with PRs that I consider uncontroversial, but for #205 I have no idea about that / whether it could break some people. The same applies to this PR, I'm afraid.

These changes also likely warrant a minor version bump since they alter the default output format - but I'm leaving it out of the pr since I'm unsure of what the exact roadmap is, if any.

Yeah, the version should be bumped by a maintainer when doing a release, not ahead of time.

@gtsiam
Copy link
Contributor Author

gtsiam commented Jul 3, 2021

there is nobody actively maintaining this crate

Yeah, I got that kind of feeling browsing through the commit history... However it's still in heavy use, so I think this is worth the effort.

I have no idea about that / whether it could break some people

I can remove the second commit and this shouldn't break compatibility in any way, if you prefer. As far as the public api is concerned it would only add the function Builder::format_target(bool) which, at least for me, would be very convenient. (well, and the documentation clarification)

I originally included 365ffaf, due to the comments stating willingness to do just that in the linked issues (by @KodrAus), but it would be easy to keep the current default, and opt in to the new behavior until it's convenient/decided to make it the new default:

env_logger::builder()
    .format_target(true)
    .format_module_path(false)
    .init();

@jplatte
Copy link
Contributor

jplatte commented Jul 3, 2021

You seem to be dedicated to making this work, which is nice. However, I really don't want to dedicate more than an absolute minimum of my time to maintenance here¹. @sirwindfield if you're still around it would be nice if you could decide whether / how to move forward here.

¹Mainly since I've fully switched to tracing and so am no longer using `env_logger`.

@gtsiam
Copy link
Contributor Author

gtsiam commented Jul 3, 2021

Context for this is warp::log(). I just wanted to see my requests with the target I set, instead of the generic module path.

I'd heard of tracing, but it seemed too complex/incompatible for my use-case, so I just went with env_logger. But if the maintenance situation is really that bad I'll take the time and look at switching to it (I just found out about tracing-log too, so there shouldn't be any problems).

For now I'll just leave this here. If you need me to change anything, ping me. Otherwise, I guess just let it be.

But this is a simple change, all things considered. If maintaining the crate is too time-consuming and there is a good alternative, It may be time to consider mentioning that in the readme and/or the docs...

@mainrs
Copy link
Contributor

mainrs commented Jul 7, 2021

I'm in the same boat. Uni takes most of my time right now (finals 🥳). I have to take a day to see what exatly changes behaviour-vise in this and other PRs.
I don't want to make promises that I can't hold though. I've blocked my Saturday morning next week after one of my exams so I can take a closer look.

Sorry for the lack of activity.

@mainrs mainrs self-assigned this Jul 13, 2021
@mainrs
Copy link
Contributor

mainrs commented Jul 14, 2021

Looking good to me. I'm OK with changing the default behavior. We just need to bump the minor then.

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.

Log with target instead of module path by default? Module path and target confusion
3 participants