Check for ORCID and ROR

[Aariq]

Draft PR to address #214

TODO:

• function documentation
• add tests
• extend to check if ORCIDs and RORs are valid (maybe a separate PR, depending on how eager you are to merge this one)
• make ROR check silent on success
• Currently makes the assumption that DESCRIPTION uses Authors@R (Check that package uses Authors@R in DESCRIPTION #244). Maybe not a safe assumption?

[Aariq]

When I run tests locally, some files get removed, but I won't commit those changes

[Aariq]

It'd be helpful to have more info or examples of other ways the authors field can look besides using Authors@R

[Aariq]

I'm thinking it would be best to save ORCID and ROR validation for a separate PR and review this as-is (just checking that the IDs exist).

[mpadge]

@Aariq Sorry for the inconvenience, but you need to please merge the recent changes to "main" into your PR to update the snapshot test results 👍

[Aariq]

I did run pr_merge_main(). When I run tests locally, a few .md and .yaml files get deleted—should I commit those changes?

[mpadge]

Thanks @Aariq. There seems to be a problem where you call this line:

  authors <- authors [!is_institution (authors)]

which then calls is_institution():

is_institution <- function (person) {
  is.null (person$family) & person$role %in% c ( "cph", "fnd")
}

But authors in the first call is a list, so person$family doesn't exist, but person[[i]]$family does. I''ve updated some snapshot results in main branch so they should all work fine here, but am still seeing test failures because of that. Once you've fixed that somehow, we should be good.

[Aariq]

I think that fixes it

[mpadge]

Thanks @Aariq, I think you just need to update the snapshot results again:

testthat::test_local()
testthat::snapshot_review("pkgcheck/")
testthat::snapshot_review("extra-checks/")

Accept all changes there, plus update number of checks in https://github.com/ropensci-review-tools/pkgcheck/blob/main/tests/testthat/test-list-checks.R to 24, because one more has been merged in the meantime. Then we should be good. Thanks for all the work!

[Aariq]

I get different failing tests locally than on GitHub actions and there are a bunch of unused snapshots that get deleted. Both snapshot_review() calls resulted in no snapshots to update 🤷🏻‍♂️. Results of testthat::test_local():

── Skipped tests (12) ───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
• !test_all is TRUE (3): test-check-scrap.R:28:5, test-goodpractice.R:5:1, test-pkgcheck.R:5:1
• On Mac (9): test-check-ci.R:21:5, test-check-covr.R:3:1, test-check-left-assign.R:3:1, test-check-renv.R:3:1, test-check-srr.R:3:1,
  test-extra-checks.R:8:1, test-github.R:20:5, test-github.R:65:1, test-path-fns.R:2:1

── Failed tests ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Failure (test-list-checks.R:6:5): list-checks
`chks` has length 24, not length 23.

Error (test-pkgcheck_bg.R:37:5): pkgcheck_bg() works
<callr_status_error/callr_error/rlib_error_3_0/rlib_error/error/condition>
Error: ! in callr subprocess.
Caused by error in `convert_path(path)`:
! could not find function "convert_path"
Backtrace:
    ▆
 1. └─x$get_result() at test-pkgcheck_bg.R:37:5
 2.   └─callr:::rp_get_result(self, private)
 3.     └─callr:::get_result(out, private$options)
 4.       └─throw(callr_remote_error(remerr, output), parent = fix_msg(remerr[[3]]))

[ FAIL 2 | WARN 0 | SKIP 12 | PASS 157 ]
Deleting unused snapshots:
• extra-checks/checks-extra.html
• extra-checks/checks-extra.md
• extra-checks/checks-print.md
• github/pkgcheck.yaml
• github/with_inputs.yaml
• pkgcheck/checks0.html
• pkgcheck/checks0.md
• pkgcheck/checks1.html
• pkgcheck/checks1.md

Is it OK to commit these changes?

[Aariq]

Nevermind, I've got all passing tests now after install()ing the package. I reverted the deleted snapshots for now. Let's see if it passes on GitHub.

[mpadge]

It won't pass until you've committed the updated snapshots to the PR. Also note that I'll be away for a week or so after today, but straight back on to it after that.

[Aariq]

I think I'm a bit stuck here. I finally realized that the issue was that those snapshot tests are usually skipped on macOS (my OS), so I need to un-skip them to update the snapshots. However, when I run the test, I get an error:

Error in pandoc_self_contained_html(file, file): Saving a widget with selfcontained = TRUE requires pandoc. See here to learn more https://bookdown.org/yihui/rmarkdown-cookbook/install-pandoc.html

I've installed pandoc, and I can't reproduce that error if I step through the test interactively (but this also doesn't save the snapshot). Any ideas?

It would be easier to work on this package if any necessary snapshot tests were run on all OS, at least locally.

[mpadge]

Nice workflow debugging, thanks @Aariq . I'm away for next week+. @maelle if you've got any insights in the meantime, please offer here : 😁. Otherwise I'll push the required changes to your PR once I'm back

[maelle]

@Aariq I've looked a bit at the snapshot in #275. To me even for the main branch the snapshots need updating? I'd suggest waiting until we hear more from Mark.