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

Add timezone to default logging time #856

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

Conversation

tim-x-y-z
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-x-y-z
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-x-y-z
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-x-y-z
Copy link
Author

amazing thanks - looking forward to it!

@rickythink
Copy link

any progress :P ?

@startime-h

This comment was marked as spam.

6 similar comments
@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

@startime-h

This comment was marked as spam.

Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
Repository owner deleted a comment from startime-h Jun 1, 2024
@tim-x-y-z
Copy link
Author

Any updated thoughts on this @Delgan ? :)

@Delgan
Copy link
Owner

Delgan commented Oct 5, 2024

Sorry for the delay @tim-x-y-z. 😬

I still plan to merge your PR after the next release of v0.7.3. It's just that I haven't yet found the time to implement the necessary fixes for the release to happen.

@tim-x-y-z
Copy link
Author

@Delgan no worries !
let me know if there's anything i can do to help :)

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.

4 participants