Only allow a single target_keys key value pair #90

annakrystalli · 2024-12-18T08:44:15Z

This PR resolves #89 and is related to hubverse-org/schemas#118

Specifically In v4.0.1, we are only allowing a single target_keys key value pair.

To ensure hubAdmin functionality conform to this pattern this PR:

[Adds a test that demonstrates correct identification of more that 1 target_keys key value pair when validating task.json files with validate_config()
Ensure an error is thrown when creating target_keys with more that 1 key value pair programmatically for configs using schema versions v4.0.1 and above
I also corrected some instances where there was a typo in the hubAdmin.branch option specification. This didn't affect tests though as it was setting it to the default value anyways.

~~Note: unrelated test failures still exist and will need resolving before merging~~ Found solution @zkamvar had applied in #86 (8e8ed51) and cherry picked into here!

… do not pass validation. Resolves #89

…en creating programmatically. Rsolves #89

github-actions · 2024-12-18T08:46:48Z

🚀 Deployed on https://678a6be90e4c8243b0492af7--hubadmin-pr-previews.netlify.app

annakrystalli · 2024-12-18T09:52:33Z

tests/testthat/testdata/v4.0.1-tasks-null-target_keys.json

+                    "target_id": "wk ahead inc flu hosp",
+                    "target_name": "weekly influenza hospitalization incidence",
+                    "target_units": "rate per 100,000 population",
+                    "target_keys": null,


section relevant to test

annakrystalli · 2024-12-18T09:52:54Z

tests/testthat/testdata/v4.0.1-tasks-2-target_keys.json

+                    "target_keys": {
+                        "target": "wk flu hosp",
+                        "target_metric": "rate change"
+                    },


section relevant to tests

annakrystalli · 2024-12-18T09:54:42Z

R/create_target_metadata_item.R

+#' @param target_keys named list or `NULL`. The `target_keys` value defines a
+#' single target.
+#' Should be `NULL`, in the case where the target is not specified as a task_id
+#' but is specified solely through the `target_id` argument.
+#' Otherwise, should be a named list containing a single character string element.
+#' The name of the element should match a `task_id`
+#' variable name within the same `model_tasks` object and the value should match
+#' a single value of that variable.


Although in the code I only apply the restriction to configs with version v4.0.1 and greater, I've gone ahead an just updated the docs with what's allowed going forward. Otherwise I think the docs would be too complex and prehaps confusing when I don't belivee anyone was using the feature anyways.

annakrystalli · 2024-12-18T09:55:32Z

R/create_target_metadata_item.R

+  is_gte_v4_0_1 <- hubUtils::version_gte(
+    "v4.0.1",
+    schema_version = schema_version
+  )
+  target_key_n <- length(target_keys)
+  if (is_gte_v4_0_1 && target_key_n > 1L) {
+    cli::cli_abort(
+      c(
+        "!" = "{.arg target_keys} must be a named {.cls list} of
+        length {.val {1L}} not {.val {target_key_n}}."
+      ),
+      call = call
+    )
+  }


I restricted this to the relevant versions to ensure it matches what's actually described in earlier schema

…o the default value anyways.

A weird bug just popped up and I don't know where it's coming from, but effectively, when a value with a class of "error" is passed to `encodeString()`, it fails with: Error in UseMethod("conditionMessage") : no applicable method for 'conditionMessage' applied to an object of class "error" This never used to happen, so I'm fixing it here.

annakrystalli · 2024-12-18T10:45:12Z

tests/testthat/test-validate_model_metadata_schema.R

@@ -6,7 +6,7 @@ test_that("Missing files returns an invalid config with an immediate message", {
      "File does not exist"
    )
  })
-  expect_false(out)
+  expect_false(unclass(out))


Fixes new errors from changes in testthat. Cherry picked from 8e8ed51

See e.g. https://github.com/hubverse-org/hubAdmin/actions/runs/12391309689/job/34588132394?pr=90#step:7:697

elray1

these changes look good to me. I'm commenting rather than approving for the same reasons outlined in this comment.

zkamvar

This looks cromulent to me! Approving pending the bump to major version discussed in hubverse-org/schemas#118 (comment)

R/create_target_metadata_item.R

Co-authored-by: Zhian N. Kamvar <[email protected]>

upgrade schema version to v5.0.0

codecov · 2025-01-17T14:52:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.30%. Comparing base (34f2624) to head (f04326f).
Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   89.25%   89.30%   +0.05%     
==========================================
  Files          30       30              
  Lines        2420     2432      +12     
==========================================
+ Hits         2160     2172      +12     
  Misses        260      260

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

annakrystalli added 2 commits December 18, 2024 10:32

Add test that confirms config files with 1 target_keys key value pair…

fec895a

… do not pass validation. Resolves #89

Do not allow target_keys of length more than 1 in v4.0.1 and above wh…

6192af2

…en creating programmatically. Rsolves #89

correct docstring

77a99af

annakrystalli mentioned this pull request Dec 18, 2024

Restrict target_keys object to contain only a single key value pair. hubverse-org/schemas#118

Merged

annakrystalli added 2 commits December 18, 2024 11:45

remove unnecessary branch specification

d5b8201

add test for NULL target keys in v4.0.1

3feb79b

annakrystalli marked this pull request as ready for review December 18, 2024 09:49

annakrystalli requested a review from elray1 December 18, 2024 09:49

reorganise tests

9874ab8

annakrystalli commented Dec 18, 2024

View reviewed changes

annakrystalli and others added 2 commits December 18, 2024 11:57

correct option specification. Does ot affect tests as it is setting t…

25b01c5

…o the default value anyways.

annakrystalli commented Dec 18, 2024

View reviewed changes

annakrystalli added 4 commits December 18, 2024 12:56

update NEWS

30ff1cb

try using full schema_id url to silence oldrel-1 test failures.

03659f6

See e.g. https://github.com/hubverse-org/hubAdmin/actions/runs/12391309689/job/34588132394?pr=90#step:7:697

Merge branch 'main' into ak/1-target-key-value-pair/89

54bca2e

appease lintr

bf87c58

zkamvar mentioned this pull request Dec 24, 2024

[LDR] hubverse target_keys should contain a single value reichlab/decisions#7

Merged

elray1 reviewed Jan 6, 2025

View reviewed changes

zkamvar approved these changes Jan 16, 2025

View reviewed changes

R/create_target_metadata_item.R Outdated Show resolved Hide resolved

R/create_target_metadata_item.R Outdated Show resolved Hide resolved

annakrystalli mentioned this pull request Jan 17, 2025

v5.0.0 hubverse-org/schemas#119

Merged

annakrystalli and others added 5 commits January 17, 2025 10:52

Update R/create_target_metadata_item.R

2ece0c3

Co-authored-by: Zhian N. Kamvar <[email protected]>

Update R/create_target_metadata_item.R

92f4687

Co-authored-by: Zhian N. Kamvar <[email protected]>

Apply suggestions from review

dd325b5

Change any mention of schema version v4.0.1 to v5.0.0

40d21a9

add Rsudio project id

97bdc3f

annakrystalli mentioned this pull request Jan 17, 2025

upgrade schema version to v5.0.0 #92

Merged

Merge pull request #92 from hubverse-org/ak/upgrade-to-v5.0.0

f04326f

upgrade schema version to v5.0.0

annakrystalli merged commit 580cc7e into main Jan 17, 2025
8 checks passed

annakrystalli deleted the ak/1-target-key-value-pair/89 branch January 17, 2025 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow a single target_keys key value pair #90

Only allow a single target_keys key value pair #90

annakrystalli commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

annakrystalli Dec 18, 2024

annakrystalli Dec 18, 2024

annakrystalli Dec 18, 2024

annakrystalli Dec 18, 2024

annakrystalli Dec 18, 2024

elray1 left a comment

zkamvar left a comment

codecov bot commented Jan 17, 2025

Only allow a single target_keys key value pair #90

Only allow a single target_keys key value pair #90

Conversation

annakrystalli commented Dec 18, 2024 • edited Loading

github-actions bot commented Dec 18, 2024 • edited Loading

annakrystalli Dec 18, 2024

Choose a reason for hiding this comment

annakrystalli Dec 18, 2024

Choose a reason for hiding this comment

annakrystalli Dec 18, 2024

Choose a reason for hiding this comment

annakrystalli Dec 18, 2024

Choose a reason for hiding this comment

annakrystalli Dec 18, 2024

Choose a reason for hiding this comment

elray1 left a comment

Choose a reason for hiding this comment

zkamvar left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2025

Codecov Report

annakrystalli commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading