BOT: Fix #684: Delete unused check_input_sample/quantile/interval functions#1093
BOT: Fix #684: Delete unused check_input_sample/quantile/interval functions#1093nikosbosse wants to merge 3 commits intomainfrom
Conversation
These three internal functions were dead code — never called in production, only tested. Their assert_input_* counterparts remain and are used by all scoring functions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1093 +/- ##
==========================================
- Coverage 97.98% 97.97% -0.01%
==========================================
Files 37 37
Lines 1984 1976 -8
==========================================
- Hits 1944 1936 -8
Misses 40 40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nikosbosse
left a comment
nikosbosse
left a comment
There was a problem hiding this comment.
CLAUDE: Clean, minimal deletion of three unused internal check_input_* wrapper functions (check_input_sample, check_input_quantile, check_input_interval). All were dead code — never called in production, not exported, only tested. The assert_input_* counterparts are correctly preserved. check_try() is correctly left intact. Tests match all 7 specifications: removal confirmation, regression guards for assert_input_*, and end-to-end scoring validation. No scope creep. Verdict: approve.
nikosbosse
changed the title
|
We don't to test that the function isn't defined anymore. Other than that looks good to me. |
Address review feedback: these tests are unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| test_that("check_input_sample() works as expected", { | ||
| # expect no error if dimensions are ok | ||
| expect_true(check_input_sample(1:10, matrix(1:20, nrow = 10))) | ||
| test_that("assert_input_sample still works after check_input_sample removal", { |
There was a problem hiding this comment.
Do we still need this in our tests now this is working? It looks like both of these things are tested elsewhere.
|
|
||
|
|
||
| test_that("check_input_interval() works as expected", { | ||
| test_that("assert_input_interval still works after check_input_interval removal", { |
There was a problem hiding this comment.
Same point here do we need this?
seabbs
left a comment
seabbs
left a comment
There was a problem hiding this comment.
I think this looks good but I don't think we need to keep these tests around as they seem to duplicate tests we already have.
2 participants
Summary
check_input_sample(),check_input_quantile(), andcheck_input_interval()check-input-helpers.Rexplicitly marks them for future deletionassert_input_*counterparts remain and are used by all scoring functionsWhat was changed
R/metrics-sample.R: Removedcheck_input_sample()definition and roxygen blockR/metrics-quantile.R: Removedcheck_input_quantile()definition and roxygen blockR/metrics-interval-range.R: Removedcheck_input_interval()definition and roxygen blockman/: Deleted 3 corresponding.Rdfilesassert_input_*+ end-to-end scoring validationTest plan
assert_input_*functions still work correctlycheck_input_*wrappersR CMD checkpasses (0 errors, 0 warnings, 2 pre-existing notes)Closes #684
🤖 Generated with Claude Code