Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

lwjohnst86
Copy link
Member

Closes #92.

Adding the include_gld_purchases() function. For adding the logic to the algorithm 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 🥳

Copy link
Collaborator

@Aastedet Aastedet left a 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", {
Copy link
Collaborator

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).

Copy link
Collaborator

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.

R/include-gld-purchases.R Outdated Show resolved Hide resolved
R/include-hba1c.R Show resolved Hide resolved
R/include-gld-purchases.R Show resolved Hide resolved
Copy link
Contributor

@signekb signekb left a 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 :)

Copy link
Member Author

@lwjohnst86 lwjohnst86 left a 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.

R/include-gld-purchases.R Show resolved Hide resolved
R/include-gld-purchases.R Outdated Show resolved Hide resolved
R/include-gld-purchases.R Outdated Show resolved Hide resolved
R/include-gld-purchases.R Outdated Show resolved Hide resolved
@lwjohnst86
Copy link
Member Author

/document

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

Create function include_gld_purchases()
3 participants