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

Use timestamp #51

Closed
wants to merge 2 commits into from
Closed

Use timestamp #51

wants to merge 2 commits into from

Conversation

FoamyGuy
Copy link
Contributor

adds logging.useTimestamp(True) which will use time.localtime() and a formatted timestamp in the emitted records instead of the default time.monotonic()

As a future improvement it could support more configuration of formatting for the timestamp which I believe the CPYthon implementation does have.

I'm not 100% certain that the way I've implemented this functionality is the best. I had initially hoped to add it as an argument to the constructor for the handler. But it turns out that the function _logRecordFactory() contains the initial creation of the record which is one spot where it needs to choose what type of value to use. Since that function is outside of the classes by itself I couldn't figure out a way to use an argument like I had hoped.

@tekktrik tekktrik self-requested a review June 14, 2023 23:19
@tekktrik
Copy link
Member

Looking at this again this week! I do think there's a way to do this on a per-handler basis that I can try out and suggest if it works. But this is a super helpful PR to have this option!

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Aug 5, 2024

@tekktrik or any @adafruit/circuitpythonlibrarians have a moment to take a look at this?

I have a project that I'm including this functionality in that I'd love to be able to use an official build of this library for instead of my modified branch.

I'm willing to make any changes that are desired in order to get it moved in.

@FoamyGuy FoamyGuy requested a review from a team August 5, 2024 14:47
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please match CPython's API.

@@ -92,6 +92,7 @@ def write(self, buf: str) -> int:
"StreamHandler",
"logger_cache",
"getLogger",
"useTimestamp",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look a like a standard CPython API because it isn't here: https://docs.python.org/3/library/logging.html#module-level-functions

How would you do it in CPython? Please match that.

@FoamyGuy
Copy link
Contributor Author

FoamyGuy commented Aug 5, 2024

Closing in favor of #61

@FoamyGuy FoamyGuy closed this Aug 5, 2024
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.

3 participants