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

feat: separate and enhance logging capabilities. #69

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

Conversation

pac-rirl
Copy link

@pac-rirl pac-rirl commented Aug 12, 2024

I have completely and blatant RIPPED-OFF your logging and I am using (the modification) in some of my own projects.

This PR separates out the logging into its own script (via source) and introduces some additional functionalities.

  1. Adding an ISO8601 Local TZ timestamp (with ms) to each of the log entries
  2. Add functionality that allows the use of 'conditional logging' blocks.

Additional block logging

# Supports the following usage:
# is_INFO, is_WARN, is_DEBUG, and is_FATAL
if is_DEBUG; then
  # a block of things to log at LOG_LEVEL_DEBUG
   DEBUG  "1) inside the DEBUG logging block"
   DEBUG "2) inside the DEBUG logging block"
fi

In the spirit of the open source community I am want to contribute some thing back.

Copy link
Author

@pac-rirl pac-rirl left a comment

Choose a reason for hiding this comment

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

I hope you might find these contributions useful. I also want to thank you.

@carlocorradini
Copy link
Owner

I will check it later because I am currently on vacation ⛱️☀️🍹

@pac-rirl
Copy link
Author

Thank you > no rush.
I do want to check with you that its okay by you that I use the inline.sh in some applications I work on.

@carlocorradini
Copy link
Owner

@pac-rirl
First of all, thank you for your contribution 🥳
However, I have a few questions or clarifications:

  1. Why do you want to separate logging from the main script?
    This is a utility script that should not have "dependencies" to prevent unnecessary complexity (use the script as is).
    If we really want to add "sources" (source or .), we need to write an appropriate CI/CD to inline the inline.sh script itself (like compiling the compiler with itself), and then create a release with the inlined script.
  2. Inline is POSIX compliant.
    However, inline-logging.sh contains #!/usr/bin/env bash, which implies BASH:
    #!/usr/bin/env bash
  3. I like the concept of using is_[LOGGING_LEVEL] to simply determine whether the logging level is enabled.
    To be consistent with the naming of other logging functions, I want to prepend it with log (log_is_FATAL or log_is_fatal).
  4. When logging messages, I like the idea to use ISO8601 and the local timezone/timestamp.
    We need to check if most systems support or have date +%FT%T.%3NZ; otherwise, disable printing ISO8601 timestamps.
    Additionally, include a configuration flag to turn it off (it's on by default).

Thanks 🤗
Let me know what you think 🤯

@carlocorradini
Copy link
Owner

@pac-rirl Any update?

@pac-rirl
Copy link
Author

pac-rirl commented Oct 28, 2024 via email

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.

2 participants