Skip to content

Conversation

@Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Aug 14, 2024

  • First commit creates an internal function containing checks common to all the cfr_() function
  • Second commit uses a collection so that the input data is completely validated in one go, and an error message with all the issues is returned. This prevents the user from having to try and error multiple times, with a different error message each time.

@Bisaloo Bisaloo requested a review from adamkucharski August 14, 2024 08:35
@adamkucharski
Copy link
Member

Thanks. Will review and merge into the latest 0.1.2 release PR #168 is complete (as seems like this is probably best sequence given CRAN submission was in limbo while this PR was added).

Copy link
Member

@adamkucharski adamkucharski left a comment

Choose a reason for hiding this comment

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

I've reviewed these changes with a mis-specified data input column, i.e. ebola1976_first_30 <- rename(ebola1976_first_30,dates=date) and found the following issue:

The original check format stops at the assert_names step with an informative warning, preventing any further checks being evaluated:

checkmate::assert_names(
colnames(data),
must.include = c("date", "cases", "deaths")
)
Error: Assertion on 'colnames(data)' failed: Names must include the elements {'date','cases','deaths'}, but is missing elements {'date'}.

However, the new function seems to hit an error because it stores the output of this check along the way, but then reaches the assert_data_frame check, which can't be completed:

.check_input_data(data)
Error in `[.data.frame`(data, , c("date", "cases", "deaths")) : 
  undefined columns selected

@Bisaloo
Copy link
Member Author

Bisaloo commented Feb 11, 2025

I see two potential fixes:

  • keep the collection approach and make sure all errors can be collected without "catastrophic" failure. This would require complexifying the code a bit but I think it's doable
  • revert to the previous approach of sequential checks & failures. This PR would thus only contain 734df58 (#161).

What would you prefer?

@Bisaloo
Copy link
Member Author

Bisaloo commented Jun 25, 2025

Hi @adamkucharski, do you see value in this?

If so, I'd welcome your input on the question above, so I can finalize this PR and get it ready to be merged.

If not, I will close this PR and delete this branch.

@adamkucharski
Copy link
Member

I won't have time to review this week, so fine to just include 734df58 (#161) if no errors here.

@Bisaloo Bisaloo force-pushed the refactor-input-checking branch from 80f57ea to a495062 Compare June 26, 2025 09:36
@Bisaloo Bisaloo force-pushed the refactor-input-checking branch from a495062 to fcffb72 Compare June 26, 2025 09:42
@Bisaloo Bisaloo requested a review from adamkucharski June 26, 2025 09:52
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.

3 participants