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

filter task groups on target rather than getting indexes with target #7

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

elray1
Copy link
Contributor

@elray1 elray1 commented Dec 23, 2024

This PR does a piece of refactoring that we identified as a nice-to-have in the review of #5.

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

Looks like a much better solution! I added a suggestion for a bit extra, but this is definitely good to go as-is!

filter_task_groups_to_target <- function(task_groups, target_id) {
# For each task group, subset the target_metadata to just the entry for the target_id
# If the target_id is not in the task group, the target_metadata will be empty
task_groups <- purrr::map(
task_groups,
function(task_group) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a much better second iteration! I would even go so far as to say that you could move this anonymous function into a named function (maybe trim_to_target_id).

Comment on lines +11 to +13
task_group$target_metadata <- purrr::keep(task_group$target_metadata,
~ .x$target_id == target_id)
return(task_group)
Copy link
Member

Choose a reason for hiding this comment

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

instead of returning task groups with empty target metadata, it might be better to return an empty list() or NULL. This way, you can use task_groups[lengths(task_groups) > 0] to filter the output later instead of using another purrr command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions! (I didn't know the lengths function before)

I'm going to merge this as is so that I can keep moving on other pieces

@elray1 elray1 merged commit 269a97e into main Dec 23, 2024
7 of 8 checks passed
@elray1 elray1 deleted the elr/refactor_task_subsetting branch December 23, 2024 20:25
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.

2 participants