Skip to content

Add Value Level Metadata support for get_control_term - #153

Open
LiamHobby wants to merge 19 commits into
mainfrom
152-get-ct-vlm
Open

LiamHobby wants to merge 19 commits into
mainfrom
152-get-ct-vlm

Conversation

@LiamHobby

Copy link
Copy Markdown
Collaborator

Summary
Enhances the get_control_term() function to support Value-Level Metadata (VLM), allowing users to retrieve control terminology conditionally based on specific VLM conditions. Previously, the function returned a single codelist per variable; now it can handle variables with multiple codelists defined by different VLM conditions via a new where parameter.

Key Changes
Added optional where parameter to get_control_term() to filter by VLM conditions
When where = "all", returns all codelists as a named list for variables with multiple VLM conditions
Improved error handling with more descriptive, user-friendly error messages using cli functions

Refactored majority of the function to control the flow depending on the scenario.

Testing:

New test cases covering:
Missing variable validation
VLM condition filtering (valid and invalid conditions)
Multiple codelists and where = "all" behavior
Dataset filtering with and without VLM
Edge cases (no control terminology, invalid codelist structure)
Added test specification file (vlm_test_spec.xlsx) with VLM test data
Created setup file (setup.R) to initialize shared test fixtures

Migration Guide for Reviewers
Backwards Compatibility: The function remains backwards compatible for non-VLM variables. Existing calls like get_control_term(meta, VAR, DS) continue to work unchanged.

New Capability: Variables with multiple codelists (VLM) previously threw an error; now users can:

Specify the exact condition: get_control_term(meta, VAR, DS, where = "CONDITION")
Get all conditions: get_control_term(meta, VAR, where = "all")

@LiamHobby LiamHobby self-assigned this May 30, 2026
@LiamHobby LiamHobby added the enhancement New feature or request label May 30, 2026
@codecov

codecov Bot commented May 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 91.86%. Comparing base (062ec1d) to head (89b7dca).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
R/metacore.R 98.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
+ Coverage   91.69%   91.86%   +0.17%     
==========================================
  Files           9        9              
  Lines        1421     1451      +30     
==========================================
+ Hits         1303     1333      +30     
  Misses        118      118              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LiamHobby
LiamHobby marked this pull request as ready for review May 30, 2026 18:07
@LiamHobby
LiamHobby requested a review from jeffreyad May 30, 2026 18:07
@jeffreyad
jeffreyad requested a review from Copilot June 3, 2026 15:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Enhances get_control_term() to support Value-Level Metadata (VLM) via a new where parameter, enabling conditional control terminology retrieval and optionally returning all VLM-specific codelists.

Changes:

  • Refactors get_control_term() to support VLM filtering (where = <condition>) and multi-condition retrieval (where = "all"), with updated cli-based errors/messages.
  • Expands test coverage for VLM/non-VLM scenarios and adds shared test fixtures via tests/testthat/setup.R and a new VLM Excel spec.
  • Updates package metadata/docs (imports, namespace, Rd, version bump, NEWS header) for the new functionality.

Reviewed changes

Copilot reviewed 7 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
tests/testthat/test-metacore.R Replaces/expands tests to cover VLM, dataset disambiguation, error cases, and invalid codelist structures.
tests/testthat/setup.R Adds shared test fixtures (p21_spec, suppae_spec, vlm_spec) for reuse across tests.
R/metacore.R Main refactor adding where support and revised control-term selection logic + messaging.
R/metacore-package.R Updates roxygen imports to include new functions used by the refactor (ensym, pmap, str_glue).
NEWS.md Adds a development-version header section.
NAMESPACE Adds generated imports for purrr::pmap and rlang::ensym (and ensures stringr::str_glue import exists).
man/spec_to_metacore.Rd Formatting-only changes in examples indentation.
man/get_control_term.Rd Updates usage/args/description/value section for the new where parameter.
DESCRIPTION Bumps package version to 0.3.0.9000.
Files not reviewed (1)
  • man/get_control_term.Rd: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread R/metacore.R
Comment thread R/metacore.R Outdated
Comment on lines +576 to +578
if (length(unique(value_spec$dataset)) > 1) {
cli_abort("{.val {var_str}} does not have a unique codelist. Please specify a dataset.")
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Implemented suggestion

Comment thread R/metacore.R
Comment thread R/metacore.R Outdated
Comment on lines +548 to +549
#' @return
#' A vector, a tibble, or NULL

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated the return types

Comment thread man/get_control_term.Rd
Comment on lines 19 to 21
\value{
a vector for permitted values and a 2-column tibble for codelists
A vector, a tibble, or NULL
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Updated automatically via devtools::document()

Comment thread R/metacore.R
Comment on lines 535 to +547
#' Get Control Term
#'
#' Returns the control term (a vector for permitted values and a tibble for code
#' lists) for a given variable. The dataset can be optionally specified if there
#' is different control terminology for different datasets
#'
#' @param metacode metacore object
#' @param variable A variable name to get the controlled terms for. This can
#' either be a string or just the name of the variable
#' @param dataset A dataset name. This is not required if there is only one set
#' of control terminology across all datasets
#'
#' @return a vector for permitted values and a 2-column tibble for codelists
#' lists) for a given variable. If the variable has value-level metadata (VLM),
#' a specific VLM condition can be selected via the `where` parameter, or
#' `where = "all"` can be used to retrieve all controlled terminology.
#'
#' @param metacode A metacore object
#' @param variable A variable name (bare or string)
#' @param dataset Optional dataset name (bare or string)
#' @param where Optional VLM condition matching `value_spec$where`, or `"all"`
#' to retrieve all control terminology for variables with multiple VLM conditions
#'

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have added the examples back in.

Comment thread R/metacore.R
Comment on lines +552 to 555
get_control_term <- function(metacode, variable = NULL, dataset = NULL, where = NULL) {
if (missing(variable)) {
cli_abort("{.var variable} must be provided.")
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

After testing this autofix suggestion is actually not valid. Because the function accepts var as both string and unquoted column name if we implement the fix as suggested is.null(variable) will attempt to resolve an unquoted column name when supplied and lead to the following error:
image

In an ideal world this function would accept only a single type for var (ideally string) but I don't wan to make this change yet as investigation would be needed onto the effects and I don't want to break people's workflow who are using unquoted names.

I think it's very unlikely people will pass "NULL" themselves so think this risk is worth accepting.

LiamHobby and others added 6 commits June 6, 2026 08:12
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug/Enhancement: get_control_term throws error when trying to get CT for a variable with VLM

2 participants