-
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_gld_purchases()
#138
base: main
Are you sure you want to change the base?
Conversation
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! I added a couple of comments to handle a few details.
dplyr::rename(date = eksd) |> | ||
dplyr::relocate(atc, .after = date) | ||
|
||
test_that("dataset needs expected variables", { |
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.
The algorithm needs either the "name" or "vnr" variable to work, so we should add this to the tests and the inclusion function itself (e.g. check for any of the variables in the input data and use the one that is available (and use "name" by default if both are available).
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.
After testing, it turns out we don't need "name" nor "vnr" any more, so this comment is irrelevant now.
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 looks good to me, but Anders' comments will need to be addressed before this can be merged :)
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.
Just some comments to @Aastedet.
Co-authored-by: Luke W. Johnston <[email protected]>
Co-authored-by: Signe Kirk Brødbæk <[email protected]>
/document |
Closes #92.
Adding the
include_gld_purchases()
function. For adding the logic to thealgorithm
dataframe, I used the symbol=~
to indicate it is a regex, since that's how it was done in Perl and it's the only other symbol I could find to represent regexes.Several files were updated, but mainly because I re-ran the targets pipeline☺️
Please, focus mainly on the tests and the function itself 🥳