-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: ✨ add include_podiatrist_services()
with tests
#182
Conversation
This function is used to convert variables like `honuge` in the format `yyww` to dates in the format `YYYY-MM-DD`.
The podiatrist services logic is needed for `include_podiatrist_services()`.
R/yyww-to-date.R
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole thing feels a bit hacky. Let me know if this isn't clear or you have another idea of how to do this.
This ensures that numeric input is converted to zero-padded characters of length 4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some suggestions.
_targets/meta/meta
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add this file to the git ignore, doesn't add anything of value to the git repo.
@@ -0,0 +1,28 @@ | |||
test_that("conversion works when 01-01 is Monday in week 1", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this message when compared to what is tested. What does 01-01
mean?
expect_equal(yyww_to_yyyymmdd("2439"), lubridate::ymd("2024-09-23")) | ||
}) | ||
|
||
test_that("conversion works when 01-01 is Friday in week 52 of the prior year", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
expect_equal(yyww_to_yyyymmdd("9307"), lubridate::ymd("1993-02-15")) | ||
}) | ||
|
||
test_that("conversion works for week 53", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add another test for when week number is 00
or 01
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I only have one minor suggestion.
Co-authored-by: Luke W. Johnston <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes, mostly around documentation/comments. But I figured out the issue with the Arrow test failing.
# Calculate the first day of the ISO year, which is when the first week | ||
# has most of the week days in it (4th of January, or the first Thursday). | ||
# See: https://en.wikipedia.org/wiki/ISO_week_date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some clarification comments, since it confused me what the code was actually doing.
dplyr::mutate( | ||
speciale = as.character(.data$speciale), | ||
barnmak = as.integer(.data$barnmak) | ||
) |> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrow needed this forcing to be character/integer in order for the filtering to work correctly.
#Conflicts: # R/sysdata.rda # _targets/meta/meta
Description
This PR adds:
algorithm.csv
include_podiatrsit_services()
functionhonuge
variable to a date (yyww_to_yyyymmdd()
) + tests for this functionlubridate
package to our dependencies (needed for handling the date conversion)I also reran the pipeline to include the podiatrist services logic.
Closes #90
Checklist
just run-all
(did not run this)