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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timezone to default logging time #856

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tim-habitat
Copy link

Hi thanks for the great library! 馃

Started using it recently, and having resources in various locations I thought it would be great to have the timezone information in the default format.
I realised that i could define a custom format for my need - but everything else just works out of the box, it felt that just this could be a nice default to have too.

Let me know your thoughts.

@Delgan
Copy link
Owner

Delgan commented May 5, 2023

Hi @tim-habitat. Thanks for the improvement proposal. 馃憤

I do like the idea of displaying the timezone by default since it eliminates any potential ambiguities. It feels good, indeed.

On the other hand, I also appreciate the simplicity of the current format. Repeatedly displaying the timezone is not necessarily useful for the default user, especially when Loguru is used as a replacement of print().

Basically, I see two cases where the timezone is very valuable:

  • reviewing logs from a system running on a different location
  • parsing logs from a file and converting them into a structured format

However, these are not necessarily common use cases and are intended for the more advanced user. So it would make sense to leave the format without timezone by default, since it is a bit less cumbersome to read.

Considering that many libraries and tools do not display the timezone by default, and that this may be what the end user prefers, I would rather leave the format as is. But in the end, I have mixed feelings.

On a side note, there is also the question of the default format used for file names. Currently, the naive local time is used. It is difficult to consider displaying the timezone, since "+" in the file name raises concerns about portability and usability. Maybe it could be left as is or converted to UTC with "Z" suffix.

@tim-habitat
Copy link
Author

Hey,

Thanks for the reply!

I take the point that not everyone would want that timezone info in their logs - particularly as it may be redundant during local development.
Though, personally, i feel it's good to always attach timezones to times and introduce this way of thinking may guide people towards the right direction before shooting themself in the foot.
Of course, i understand loguru achieved the level of popularity because of its ease of use and all. This was more of a selfish suggestion as i felt all the other default were perfect (even at "scale").

On the filename, i think converting to UTC would make the most sense - but this needs to be easily understood by the users for the same reasons. I don't really see any other way either to include an iso 8601 name.

@Delgan
Copy link
Owner

Delgan commented May 8, 2023

Though, personally, i feel it's good to always attach timezones to times and introduce this way of thinking may guide people towards the right direction before shooting themself in the foot.

Yes, that's totally how I feel as well.
The user may have unpleasant surprises the day he needs to manipulate the logged "naive" dates. Whereas with "aware" date by default, a whole lot of possible problems are avoided.

I agree with you about enforcing good practices, even if it can be a bit annoying visually, it shouldn't be a big deal.

I'm leaving this open for now, I could probably make it the default for the next version, thanks. 馃憤

@tim-habitat
Copy link
Author

hey @Delgan - did you have anymore thoughts about this ? :)

@Delgan
Copy link
Owner

Delgan commented Sep 7, 2023

I'm waiting for the next minor release, namely v0.8.0, to merge this, as this is kind of a breaking change.
I need first to publish v0.7.2 due to a bug introduced in v0.7.1.

@tim-habitat
Copy link
Author

amazing thanks - looking forward to it!

@rickythink
Copy link

any progress :P ?

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

3 participants