deprecation(groupComparisonPTM): Deprecate data.type parameter#124
deprecation(groupComparisonPTM): Deprecate data.type parameter#124tonywu1999 merged 3 commits intodevelfrom
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/groupComparisonPTM.R (1)
114-117:⚠️ Potential issue | 🟡 MinorInconsistent
use_log_fileargument between PTM and proteingroupComparisoncalls.Line 146 passes
use_log_fileas a positional argument togroupComparisonfor the protein model, but the equivalent PTM call on line 114-115 does not. This means the PTM LF path omitsuse_log_filewhile the protein LF path includes it. Either both calls should pass it or neither should.Also applies to: 144-148
🤖 Fix all issues with AI agents
In `@R/groupComparisonPTM.R`:
- Around line 58-59: Validate the incoming ptm_label_type and protein_label_type
using match.arg (e.g., allowed = c("TMT","LF")) at the start of the function so
invalid values error early with a clear message; then use the validated
variables when building ptm_model_full and protein_model_full so those objects
are always defined for the subsequent branching logic (referencing
ptm_label_type, protein_label_type, ptm_model_full, protein_model_full). Ensure
you replace any raw string comparisons with the validated variables and do not
proceed to construct models unless match.arg succeeded.
🧹 Nitpick comments (2)
R/groupComparisonPTM.R (2)
85-85: Significant amount of commented-out logging code left behind.There are ~15 blocks of commented-out logging calls (
getOption(option_log), log_file_path references) scattered throughout the function. Consider removing them to reduce noise, or if logging is planned for re-introduction, track it in an issue instead.Also applies to: 99-99, 105-108, 113-113, 116-117, 130-130, 136-139, 143-143, 147-148, 157-157, 162-162, 169-170, 209-209
56-68: Roxygen@paramorder doesn't match the function signature.
ptm_label_typeandprotein_label_typeare the 3rd and 4th parameters in the signature but are documented afterbasein the roxygen block (lines 43-46). While roxygen2 handles this, matching the order improves readability.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/groupComparisonPTM.R (1)
60-72:⚠️ Potential issue | 🟠 MajorBreaking change disguised as deprecation —
data.typeremoved without a deprecation path.The PR title says "Deprecate data.type parameter," but the parameter is fully removed. Any downstream code calling
groupComparisonPTM(..., data.type = "TMT")will fail immediately with an unexpected-argument error. A proper deprecation would retaindata.typewith a warning, map it to the new parameters, and schedule removal for a future release.Suggested deprecation shim (add to function body after match.arg calls)
groupComparisonPTM = function(data, contrast.matrix = "pairwise", ptm_label_type = c("LF", "TMT"), protein_label_type = c("LF", "TMT"), moderated = FALSE, adj.method = "BH", log_base = 2, save_fitted_models=TRUE, use_log_file = TRUE, append = FALSE, verbose = TRUE, log_file_path = NULL, - base = "MSstatsPTM_log_") { + base = "MSstatsPTM_log_", + data.type = deprecated()) { Label = Site = NULL + + if (lifecycle::is_present(data.type)) { + lifecycle::deprecate_warn( + "2.x.0", + "groupComparisonPTM(data.type)", + "groupComparisonPTM(ptm_label_type)" + ) + ptm_label_type = data.type + protein_label_type = data.type + } + ptm_label_type = match.arg(ptm_label_type) protein_label_type = match.arg(protein_label_type)
🧹 Nitpick comments (1)
R/groupComparisonPTM.R (1)
91-91: Clean up commented-out logging code.There are ~15 blocks of commented-out
getOption(option_log)(...)and related logging calls scattered throughout the function. Since the logging scaffolding has been intentionally removed, these remnants add noise. Consider removing them in this PR to keep the diff clean for future readers.Also applies to: 105-105, 111-114, 119-119, 136-136, 142-145, 149-149, 153-154, 163-163, 168-168, 175-176, 215-215
1 participant
Summary by CodeRabbit
Breaking Changes
Documentation