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

Adds timestamps to the log format #11

Merged
merged 1 commit into from
Nov 5, 2020

Conversation

SirMoM
Copy link
Contributor

@SirMoM SirMoM commented Oct 9, 2020

Add timestamps to be used in the log format.
And updates the README.md to reflect the changes and give more insight to the formatting options.

Example:

var msg: String = "Error occurred!"

Logger.get_module("main").time_template = "dd.mm.yyyy hh:MM:ss"
Logger.error(msg)

Logger.get_module("main").time_template = "hh:MM:ss"
Logger.error(msg)

Logger.output_format = "[{LVL}] {MSG} at {TIME}"
Logger.error(msg)

Results in:
log_formating

I mentioned a timestamp system in #9

@SirMoM SirMoM marked this pull request as ready for review October 9, 2020 11:11
@SirMoM
Copy link
Contributor Author

SirMoM commented Oct 22, 2020

Is there any problem with the pull request?
Or are you just busy @akien-mga ?
I don't want to be annoying but I'm a bit confused why this PR is halted 😬

@akien-mga
Copy link
Member

Is it really necessary to make this a per-module configuration, instead of having it global like output_format? To me both output_format and this new time_template serve a similar purpose, I'm not sure why you would want to have a single output format with different timestamp formats based on the module.

I also have style changes to make to the added documentation, I'll push them directly as a commit to this PR once we agree on making this global or module-specific.

logger.gd Show resolved Hide resolved
@akien-mga
Copy link
Member

akien-mga commented Nov 4, 2020

Here's my proposal to simplify the API and improve the docs and formatting: akien-mga@3dfd241

If you're OK with the changes I'll squash the two commits and push to update this PR and merge.

@akien-mga
Copy link
Member

Also, did you follow any specific standard for the timestamp placeholders?

Going by https://en.wikipedia.org/wiki/ISO_8601 (which seems to match Unix timestamp), these would be better:

YYYY
MM
DD

hh
mm
ss

@akien-mga
Copy link
Member

akien-mga commented Nov 4, 2020

New commit: https://github.com/akien-mga/godot-logger/tree/SirMom-feature/log-timestamp

Edit: Updated after actually testing my changes and finding some issues.

@SirMoM
Copy link
Contributor Author

SirMoM commented Nov 4, 2020

Is it really necessary to make this a per-module configuration, instead of having it global like output_format? To me both output_format and this new time_template serve a similar purpose, I'm not sure why you would want to have a single output format with different timestamp formats based on the module.

I also have style changes to make to the added documentation, I'll push them directly as a commit to this PR once we agree on making this global or module-specific.

I did that because I thought the purpose of the modules is the configuration of the log message and thus I put in the Module.
But if we ditch the modules altogether then this is fine, but this is up for discussion in #13.

@SirMoM
Copy link
Contributor Author

SirMoM commented Nov 4, 2020

Also, did you follow any specific standard for the timestamp placeholders?

Going by https://en.wikipedia.org/wiki/ISO_8601 (which seems to match Unix timestamp), these would be better:

YYYY
MM
DD

hh
mm
ss

No I did not follow any standard but had this in mind. Yes following this is a way better approach!

@akien-mga
Copy link
Member

I did that because I thought the purpose of the modules is the configuration of the log message and thus I put in the Module.

Modules are not specifically for configuration, but to have different channels for different types of log messages. E.g. in a networked game you could have a "Server" module and a "Client" module, etc.

I'm not against making configuration module-specific, but that adds configuration overhead every time you create new modules (and this would require making output_format a module specific option too, otherwise it doesn't make much sense to configure e.g. the timestamp format here if you can't configure the output format on a module level).

Timestamps follow ISO 8601.
Fix some pre-existing bogus docs.

Co-authored-by: Rémi Verschelde <[email protected]>
@akien-mga akien-mga merged commit f4cdc74 into KOBUGE-Games:master Nov 5, 2020
@akien-mga
Copy link
Member

Thanks!

@SirMoM SirMoM deleted the feature/log-timestamp branch November 5, 2020 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants