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

Updating get_meta to handle 3-col info on filters and indicators #33

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

rmbielby
Copy link
Contributor

@rmbielby rmbielby commented Oct 3, 2024

Brief overview of changes

The meta data returned by the API has been changed to include id, col_name and label for filter and indicator columns (where previously it was just col_name and label). This updates get_meta() to account for the change to the API behaviour.

Why are these changes being made?

Change in API behaviour

Detailed description of changes

I've updated parse_meta_filter_columns() to account for the extra column and to label them up appropriately in the returned meta data frame.

Bonus change: I originally wrote parse_meta_indicator_columns() for get_meta(), but realised that parse_meta_filter_columns() did an identical job, so never actually used it. I intended to go back and delete it before finishing get_meta() but must have forgotten, so done that now.

Also updated the tests to account for the change.

@rmbielby rmbielby added the enhancement Improvement to existing feature label Oct 3, 2024
@rmbielby rmbielby added this to the Ready for user testing milestone Oct 3, 2024
@rmbielby rmbielby self-assigned this Oct 3, 2024
tests/testthat/seed_tests.R Fixed Show fixed Hide fixed
@rmbielby rmbielby requested a review from cjrace October 3, 2024 08:25
Copy link
Contributor

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

One minor comment for somewhere else I think you need to update - should this be col_id rather than col_name?

image

@cjrace
Copy link
Contributor

cjrace commented Oct 3, 2024

Possibly also a question for your next work on the tidying / swapping of ids to labels as to whether having both col_id and col_name would be helpful in the meta response for filter_items. It's not necessary, but could be nice for users, and might be helpful for some of the processing (or might not make a difference!)

@rmbielby
Copy link
Contributor Author

rmbielby commented Oct 3, 2024

One minor comment for somewhere else I think you need to update - should this be col_id rather than col_name?

image

Made that change now.

@rmbielby
Copy link
Contributor Author

rmbielby commented Oct 3, 2024

Possibly also a question for your next work on the tidying / swapping of ids to labels as to whether having both col_id and col_name would be helpful in the meta response for filter_items. It's not necessary, but could be nice for users, and might be helpful for some of the processing (or might not make a difference!)

Maybe, I could argue myself either way I think. Think I'd either keep it as it is or merge filter_columns and filter_items together as one single thing. Not keen on the halfway ground of the col_names being repeated in two different places though. At the moment, I feel like it's easy enough for a user to do a left join on col_id if they want everything together, so marginally favour keeping the status quo as it feels like the most efficient way of storing the info.

@rmbielby rmbielby merged commit 0b6690a into main Oct 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants