Remove log4r dependency (replace with base R logging)#141
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe package replaces log4r appenders with internal file, console, and null appenders, updates logging configuration and package imports, documents the new helpers, bumps the version to 1.23.3, and removes log4r-specific vignette wording. ChangesInternal logging replacement
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
R/utils_logging.R (2)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape brackets in Roxygen comment to prevent unintended link generation.
Because Markdown is enabled in
roxygen2, the text[timestamp]is interpreted as a link to thetimestampfunction topic, producing\link{timestamp}in the generated.Rdfile (as seen inman/dot-formatLogMessage.Rd). Escaping the brackets prevents this unintentional formatting.🛠️ Proposed fix
-#' Format a log line as "LEVEL [timestamp] message" +#' Format a log line as "LEVEL \[timestamp\] message"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/utils_logging.R` at line 15, Update the roxygen comment describing the log format above dot-formatLogMessage to escape both brackets around timestamp, preserving the displayed “LEVEL [timestamp] message” text without allowing roxygen2 Markdown to generate a link.
39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute console logs to
stderr()instead ofstdout()
cat()writes to standard output by default, so these messages will mix with captured output and piped data. Switching this appender tofile = stderr()keeps diagnostics separate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@R/utils_logging.R` around lines 39 - 45, Update .consoleAppender so its cat() call writes formatted log messages to stderr() via the file argument, preserving the existing message formatting and single-line behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@R/utils_logging.R`:
- Around line 27-36: Update .fileAppender so append = FALSE truncates the log
file once during appender creation, then make each closure write use append =
TRUE. Preserve append = TRUE behavior and ensure multiple log calls retain all
entries.
---
Nitpick comments:
In `@R/utils_logging.R`:
- Line 15: Update the roxygen comment describing the log format above
dot-formatLogMessage to escape both brackets around timestamp, preserving the
displayed “LEVEL [timestamp] message” text without allowing roxygen2 Markdown to
generate a link.
- Around line 39-45: Update .consoleAppender so its cat() call writes formatted
log messages to stderr() via the file argument, preserving the existing message
formatting and single-line behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0f66683-9d96-4c9f-be5f-a2ef9fcc05de
📒 Files selected for processing (8)
DESCRIPTIONNAMESPACER/utils_logging.Rman/dot-consoleAppender.Rdman/dot-fileAppender.Rdman/dot-formatLogMessage.Rdman/dot-nullAppender.Rdvignettes/msstats_data_format.Rmd
💤 Files with no reviewable changes (1)
- NAMESPACE
| #' File appender: returns a function(level, ...) that writes one line to a file | ||
| #' @keywords internal | ||
| .fileAppender = function(log_file_path, append = TRUE) { | ||
| force(log_file_path) | ||
| force(append) | ||
| function(level, ...) { | ||
| cat(.formatLogMessage(level, ...), file = log_file_path, sep = "", | ||
| append = append) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the behavior of R's cat function when append=FALSE
Rscript -e '
path <- "test_log.txt"
cat("First log line\n", file=path, append=FALSE)
cat("Second log line\n", file=path, append=FALSE)
cat("File contents:\n")
cat(readLines(path), sep="\n")
'Repository: Vitek-Lab/MSstatsConvert
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file with line numbers and surrounding context.
git ls-files 'R/utils_logging.R'
wc -l R/utils_logging.R
cat -n R/utils_logging.R | sed -n '1,120p'
# Search for .fileAppender usages and related logging helpers.
rg -n "\.fileAppender|append = FALSE|fileAppender|formatLogMessage|log_file_path" RRepository: Vitek-Lab/MSstatsConvert
Length of output: 11381
Log file is truncated on every write when append = FALSE. In R/utils_logging.R:29-35, the closure passes append = append to cat() for every message, so an existing file gets reopened in truncate mode on each log call and only the last entry survives. Truncate once when the appender is created, then always write with append = TRUE.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@R/utils_logging.R` around lines 27 - 36, Update .fileAppender so append =
FALSE truncates the log file once during appender creation, then make each
closure write use append = TRUE. Preserve append = TRUE behavior and ensure
multiple log calls retain all entries.
2 participants
Motivation and Context
Please include relevant motivation and context of the problem along with a short summary of the solution.
Changes
Please provide a detailed bullet point list of your changes.
Testing
Please describe any unit tests you added or modified to verify your changes.
Checklist Before Requesting a Review
Motivation and context
The package removes its
log4rdependency and switches to base R logging, while keeping the same user-facing behaviors for writing logs to a file, emitting messages to the console, or suppressing output. The update introduces internal logging helpers and adjusts package initialization/configuration to use them instead oflog4r.Changes
DESCRIPTIONfrom1.23.2to1.23.3.log4rfromDESCRIPTIONImports.NAMESPACEto removeimportFrom(log4r, console_appender)andimportFrom(log4r, file_appender).R/utils_logging.R:.formatLogMessage()to standardize log line formatting.log4rappender usage with internal closures:.fileAppender()(writes one formatted log line to a file).consoleAppender()(writes one formatted log line to the console).nullAppender(suppresses output).onLoadto conditionally setMSstatsLog/MSstatsMsgusing the new internal appenders when not already configured.MSstatsLogsSettings()to build logging behavior using.fileAppender()/.nullAppender(file logging controlled byuse_log_file) and.consoleAppender()/.nullAppender(console logging controlled byverbose), wiring results intooptions(...)..consoleAppender(),.fileAppender(), and.formatLogMessage()..nullAppenderdocumentation wording to remove “log4r appender” phrasing.vignettes/msstats_data_format.Rmd) to describe flexible logging without referencinglog4r.docs/pkgdown HTML still contains somelog4rreferences (e.g.,docs/articles/msstats_data_format.html,docs/reference/dot-nullAppender.html), suggesting the site output may not have been fully regenerated after source changes.Unit tests
No unit tests were added or modified.
Coding guideline violations
No coding guideline violations were identified from the provided changes; however, there is a documentation consistency gap in the generated
docs/HTML wherelog4rtext remains despite source updates.