mrc 5087: Add endpoint to list parameters on a report #4
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 7 +1
Lines 111 142 +31
=========================================
+ Hits 111 142 +31 ☔ View full report in Codecov by Sentry. |
To indicate when a particular report has modifications from the default branch
R/reports.R
Outdated
| contents <- gert::git_ls(root, ref = ref) | ||
| re <- sprintf("^src/%s/(%s|orderly)\\.R$", name, name) | ||
| matches <- grep(re, contents$path, value = TRUE, perl = TRUE) | ||
| if (length(matches) > 1) { |
There was a problem hiding this comment.
should we care about the case where they have none? or assumed to have at least one at this point?
There was a problem hiding this comment.
API should only call this for directories which have an orderly.R script. So this shouldn't be triggered with matches 0. That said, better to not rely on that knowledge as the error message raised if there is no file is quite ugly. So I've expanded this check to test if != 0
There was a problem hiding this comment.
LGTM with the mrc-ide/orderly2#129 merged and DESCRIPTION here updated
DESCRIPTION
Outdated
| @@ -32,7 +32,7 @@ Suggests: | |||
| testthat (>= 3.0.0) | |||
| Config/testthat/edition: 3 | |||
| Remotes: | |||
| mrc-ide/orderly2, | |||
| mrc-ide/orderly2@add-ast-read, | |||
There was a problem hiding this comment.
This is ready to be merged now
| mrc-ide/orderly2@add-ast-read, | |
| mrc-ide/orderly2, |
DESCRIPTION
Outdated
| @@ -18,7 +18,7 @@ Imports: | |||
| gert (>= 2.0.1), | |||
| ids, | |||
| jsonlite, | |||
| orderly2, | |||
| orderly2 (>= 1.99.12), | |||
There was a problem hiding this comment.
| orderly2 (>= 1.99.12), | |
| orderly2 (>= 1.99.13), |
Note this uses mrc-ide/orderly2#129 and should be merged after that
This PR will add an endpoint to list the parameters available on a report for a particular name and hash
Builds on #3 so should be merged after that