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

fix logger error All unnamed arguments must be length 1. #103

Merged
merged 2 commits into from
Jan 14, 2025
Merged

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Jan 14, 2025

glue fails when receives a vector of length > 1. Message generate with warning could have n-elements and parse_logger_message would return a vector instead of a single character.

Example

teal.logger::register_logger("teal.code")
teal.logger::register_handlers("teal.code")
teal.code::qenv() |>
  within(a <- 1) |>
  teal.code::get_var("a")
  
# Error in h(simpleError(msg, call)) : 
#   `glue` failed in `formatter_glue` on:
#
#  Named chr [1:2] "`get_var()` was deprecated in teal.code 0.5.1." "Please use `base::get()` instead."
#  - attr(*, "names")= chr [1:2] "" "i"
#
# Raw error message:
#
# All unnamed arguments must be length 1
#
# Please consider using another `log_formatter` or `skip_formatter` on strings with curly braces.  
  

@gogonzo gogonzo added the core label Jan 14, 2025
@gogonzo gogonzo self-assigned this Jan 14, 2025
@gogonzo gogonzo requested a review from llrs-roche January 14, 2025 11:50
Copy link
Contributor

Unit Tests Summary

 1 files   4 suites   0s ⏱️
33 tests 33 ✅ 0 💤 0 ❌
46 runs  46 ✅ 0 💤 0 ❌

Results for commit e25afa4.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Unit Tests Summary

 1 files   4 suites   0s ⏱️
34 tests 34 ✅ 0 💤 0 ❌
49 runs  49 ✅ 0 💤 0 ❌

Results for commit 2e8be3a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

This solves the issue. I get a warning though:

[WARN] 2025-01-14 13:50:54.6885 pid:25376 token:[] teal.code `get_var()` was deprecated in teal.code 0.5.1.
Please use `base::get()` instead.
[1] 1

@gogonzo gogonzo requested a review from llrs-roche January 14, 2025 12:57
@llrs-roche
Copy link
Contributor

The change fixed the issues because parse_logger_message() -> register_handler_type() -> register_handlers() which is the one used to log the messages via {logger}, which logger can only handle one message at the time, while here it was handled with multiple ones.

The warning is due to teal.code::get_var() being deprecated. To avoid it one needs to do one of the three last lines:

teal.logger::register_logger("teal.code")
teal.logger::register_handlers("teal.code")
q <- teal.code::qenv() |>
  within(a <- 1) 
q$a
q[["a"]]
get("a", envir = q)

But these other methods do not trigger the logging of the deprecation message, without which we cannot get a multiple character vector as a message and see that the parse_logger_message() fails.

Please provide a test to add to the code base so that multiple messages are logged correctly from now on.

Copy link
Contributor

github-actions bot commented Jan 14, 2025

Unit Test Performance Difference

Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
register_handlers 👶 $+0.01$ parse_logger_message_concatenates_n_element_message

Results for commit 2ad660c

♻️ This comment has been updated with latest results.

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Looks good and works well!

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot to mention this. I think a version bump is in order to make other packages like tmc and tmg depend on this specific version or above (just thinking on the tmg CI failing to resolve dependencies which is something that I got recently locally)

gogonzo added a commit to insightsengineering/teal.goshawk that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.slice that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.modules.hermes that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.modules.clinical that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.modules.general that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.transform that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal that referenced this pull request Jan 14, 2025
gogonzo added a commit to insightsengineering/teal.osprey that referenced this pull request Jan 14, 2025
@gogonzo
Copy link
Contributor Author

gogonzo commented Jan 14, 2025

@llrs-roche all packages dependent on teal.logger bumped in the DESCRIPTION file. PR's linked to this PR 👍

@llrs-roche llrs-roche merged commit 536bc50 into main Jan 14, 2025
26 checks passed
@llrs-roche llrs-roche deleted the fix branch January 14, 2025 15:51
@github-actions github-actions bot locked and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants