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_tidy_github_actions() #171

Merged
merged 30 commits into from
Aug 6, 2024
Merged

use_tidy_github_actions() #171

merged 30 commits into from
Aug 6, 2024

Conversation

hadley
Copy link
Collaborator

@hadley hadley commented Aug 2, 2024

Use the latest versions of r-lib actions, and flesh out a more complete R version matrix.

Use the latest versions of r-lib actions, and flesh out a more complete R version matrix.
@hadley hadley marked this pull request as ready for review August 2, 2024 12:42
@hadley
Copy link
Collaborator Author

hadley commented Aug 5, 2024

@daroczig I can't figure out why this is failing on mac. The error occurs in this block:

log_formatter(formatter_pander)
log_info(head(iris))

And it seems like it's because it's using formatter_glue instead of formatter_pander, but I don't understand why, and I can't reproduce locally.

@hadley
Copy link
Collaborator Author

hadley commented Aug 6, 2024

I figured it out! I'm not sure why it only failed on mac, but the problem was that the loggers from previously rendered vignettes were still active, so once I implemented a more thorough reset, the problem went away 🥳

Copy link
Collaborator Author

@hadley hadley left a comment

Choose a reason for hiding this comment

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

@daroczig this is now ready for review! 😄

}
message

withCallingHandlers(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was previously incorrectly vectorised, leading to a bunch of duplication. I fixed the problem and added a test.

@@ -1,7 +1,12 @@
## init storage for all logger settings
namespaces <- new.env()
namespaces <- new.env(parent = emptyenv())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think this changes behaviour, but it's good practice.

@@ -4,6 +4,7 @@
#' @export
#' @importFrom logger log_level
logger_tester_function <- function(level, msg) {
set.seed(1014)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes the output reproducible for use in snapshot tests.

'|||||')
local_test_logger(layout = layout_glue_generator("{level} {msg}"))

expect_snapshot({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think snapshots do a better job of capturing the intent here (I'm sure they didn't exist when this code was written), and since it doesn't seem to need to be run in a separate process.

expect_match(eval_outside('system("echo 42", invisible = TRUE)'), 'INFO')
}
expect_snapshot({
writeLines(eval_outside("log_messages()", 'message(42)'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similarly I moved all these to snapshot tests, which makes it easier to see what's going wrong if there are failures.

'logger.tester / logger.tester / logger.tester / logger_info_tester_function / logger_info_tester_function("foobar")')
unlink(t)
test_that('log_info() captures local info', {
local_test_logger(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again, not sure why this was running in separate process, but the snapshot outputs from the active process look fine to me.

Copy link
Owner

Choose a reason for hiding this comment

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

it's been a long time ago 🤐 but if I recall correctly, I had some issues with testthat injecting frames in the call stack ... but happy to hear it's working 🎉

@@ -89,17 +82,13 @@ Putting all these together (by explicitly setting the default config in the `glo
log_threshold(INFO)
log_formatter(formatter_glue)
log_layout(layout_simple)
log_appender(appender_console)
log_appender(appender_stdout)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was causing log output to be lost since knitr doesn't (and IMO shouldn't) capture stderr.

Copy link
Owner

@daroczig daroczig left a comment

Choose a reason for hiding this comment

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

this is fantastic, thanks a ton -- highly appreciated 🙌

@daroczig daroczig merged commit f0ba810 into daroczig:master Aug 6, 2024
13 checks passed
@hadley hadley deleted the actions branch August 7, 2024 12:18
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