mrc-5105: Add has_modifications flag to report list endpoint #3
Conversation
r-ash
changed the title
| @@ -33,17 +33,24 @@ root <- function() { | |||
|
|
|||
| ##' @porcelain | |||
| ##' GET /report/list => json(report_list) | |||
| ##' query hash :: string | |||
| ##' query ref :: string | |||
There was a problem hiding this comment.
I changed this to ref too because I think this is better description, it can work with a hash or a branch name
r-ash
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 6 +1
Lines 65 111 +46
=========================================
+ Hits 65 111 +46 ☔ View full report in Codecov by Sentry. |
To indicate when a particular report has modifications from the default branch
As this can take any ref, a hash, a branch name etc.
R/git.R
Outdated
| } | ||
|
|
||
|
|
||
| git_ref_to_sha <- function(ref, repo = NULL, check = FALSE) { |
There was a problem hiding this comment.
Is this not gert::git_commit_id?
There was a problem hiding this comment.
It absolutely is yes, thanks!
| } | ||
|
|
||
|
|
||
| git_get_modified <- function(ref, base = NULL, |
There was a problem hiding this comment.
we should make a ticket to patch gert for this perhaps
There was a problem hiding this comment.
Agreed, added a ticket for this https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5126/Update-gert-to-be-able-to-get-diff-with-...-syntax
R/util_assert.R
Outdated
| i = "{name} has length {length(x)}"), | ||
| call = call, arg = arg) | ||
| } | ||
| } |
There was a problem hiding this comment.
consider invisible return of x on all of these (not needed on assert_scalar_character as that's implicit) as that's often useful
R/util_assert.R
Outdated
| @@ -0,0 +1,25 @@ | |||
| assert_scalar <- function(x, name = deparse(substitute(x)), arg = name, | |||
There was a problem hiding this comment.
After your change you no longer need these :) but they're bound to come in useful lter
tests/testthat/test-util-assert.R
Outdated
| test_that("assert_scalar", { | ||
| expect_error(assert_scalar(NULL), "must be a scalar") | ||
| expect_error(assert_scalar(numeric(0)), "must be a scalar") | ||
| expect_error(assert_scalar(1:2), "must be a scalar") |
There was a problem hiding this comment.
| expect_error(assert_scalar(1:2), "must be a scalar") | |
| expect_error(assert_scalar(1:2), "must be a scalar") | |
| expect_no_error(assert_scalar(1)) |
To indicate when a particular report has modifications from the default branch.
We want this because in the UI when users view the list of reports, we want to have a checkbox to show only those reports which contain modifications (if the user has selected a branch other than the default branch)
This will always return
has_modificationseven if the hash this is called with the default branch or a hash which is on the default branch.This gets the default branch from the
gert::git_remote_info("origin"). This is theHEAD branchfrom the remote origin, which if I understand correctly should always be the GitHub default branch.This gets the modified files using
git diff <from>...<to>using the remote origin as the<from>branch. This is the same as whatorderly.serveris doing at the moment. I expect we could probably make an enhancement ingertto be able to callgert::git_diff()with a specified base ref but can do that later.