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

Logger overhaul #98

Merged
merged 47 commits into from
Feb 20, 2024
Merged

Logger overhaul #98

merged 47 commits into from
Feb 20, 2024

Conversation

RasmusSkytte
Copy link
Contributor

@RasmusSkytte RasmusSkytte commented Feb 1, 2024

Intent

This PR introduces an overhaul for the Logger to make it simpler to use in different cases.

Approach

  • The Logger was re-factored

  • The LoggerNull class was added, which matches the structure of Logger but does not write logs to console, file or DB.

  • Fields that were previously public are now made private with corresponding active bindings to show their values.

  • The db_tablestring argument was replaced with db_table and made more flexible:
    i.e. it now takes id-like objects instead of only a character string.

  • The ts argument was renamed to timestamp to match update_snapshot()

  • $log_info(), $log_warn() and $log_error() were adjusted.

    • The message produced by $log_info() is now return invisibly to simplify $log_warn() and $log_error().
    • The message in $log_info() is printed with message() instead of cat() to allow the suppression of output (CRAN best practices).
  • A custom logging format for timestamps can be passed to the $log_*() functions via the argument timestamp_format or the option "SCDB.log_timestamp_format" (fixes FEATURE: Custom logger format #65).

  • The tests for Logger were made specific (ie. the single large test was broken into several tests).
    In addition extra checks are made during testing to ensure "info", "warnings" and "errors" work as intended for different combinations of logging to file and logging to console.

  • checkmate assertions improved when log_conn = NULL

  • return statements gets their own line in accordance with coding standard.

Known issues

This PR also contains the code being merged in

Once merged, this PR will be rebased and opened.

Checklist

  • The PR passes all local unit tests
  • I have documented any new features introduced
  • If the PR adds a new feature, please add an entry in NEWS.md
  • A reviewer is assigned to this PR

@RasmusSkytte RasmusSkytte self-assigned this Feb 1, 2024
@RasmusSkytte RasmusSkytte added the enhancement New feature or request label Feb 1, 2024
@RasmusSkytte RasmusSkytte changed the title Minor Logger improvements Minor Logger improvements Feb 1, 2024
@RasmusSkytte RasmusSkytte changed the title Minor Logger improvements Logger improvements Feb 5, 2024
@RasmusSkytte RasmusSkytte added this to the v0.4 milestone Feb 6, 2024
R/Logger.R Outdated Show resolved Hide resolved
R/Logger.R Outdated Show resolved Hide resolved
Copy link
Contributor

@LasseEngboChr LasseEngboChr left a comment

Choose a reason for hiding this comment

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

Only a few suggestions

This was referenced Feb 13, 2024
Copy link
Contributor

@LasseEngboChr LasseEngboChr left a comment

Choose a reason for hiding this comment

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

One line in documentation should be deleted.

R/Logger.R Outdated Show resolved Hide resolved
@RasmusSkytte RasmusSkytte merged commit e1118bc into main Feb 20, 2024
24 of 25 checks passed
@RasmusSkytte RasmusSkytte deleted the feat/Logger_updates branch February 20, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Custom logger format
2 participants