Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline and tighten lintr checks #220

Merged
merged 23 commits into from Apr 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 54 additions & 0 deletions .github/workflows/lint-changed-files.yaml
@@ -0,0 +1,54 @@
# Workflow derived from https://github.com/r-lib/actions/tree/v2/examples
# Need help debugging build failures? Start at https://github.com/r-lib/actions#where-to-find-help
on:
push:
branches:
- main
- master
- develop
pull_request:
branches:
- main
- master
- develop
workflow_dispatch:

name: lint-changed-files

jobs:
lint-changed-files:
runs-on: ubuntu-latest
if: "! contains(github.event.head_commit.message, '[ci skip]')"
env:
GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
steps:
- uses: actions/checkout@v3

- uses: r-lib/actions/setup-r@v2

- uses: r-lib/actions/setup-r-dependencies@v2
with:
extra-packages: |
any::gh
any::lintr
any::purrr
needs: check

- name: Add lintr options
run: |
cat('\noptions(lintr.linter_file = ".lintr")\n', file = "~/.Rprofile", append = TRUE)
shell: Rscript {0}

- name: Install package
run: R CMD INSTALL .

- name: Extract and lint files changed by this PR
run: |
files <- gh::gh("GET https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files")
changed_files <- purrr::map_chr(files, "filename")
all_files <- list.files(recursive = TRUE)
exclusions_list <- as.list(setdiff(all_files, changed_files))
lintr::lint_package(exclusions = exclusions_list)
shell: Rscript {0}
env:
LINTR_ERROR_ON_LINT: true
36 changes: 0 additions & 36 deletions .github/workflows/lint.yaml

This file was deleted.

22 changes: 15 additions & 7 deletions .lintr
@@ -1,7 +1,15 @@
linters: linters_with_defaults(
line_length_linter = line_length_linter(80),
cyclocomp_linter = cyclocomp_linter(complexity_limit = 20L),
object_usage_linter = NULL)
exclusions: c(list.files(path = "tests/", recursive = T, full.names = T),
"vignettes/germany-age-stratified-nowcasting.Rmd.orig")
exclude: "# nolint"
linters: linters_with_tags(
tags = NULL, # include all linters
implicit_integer_linter = NULL,
seabbs marked this conversation as resolved.
Show resolved Hide resolved
extraction_operator_linter = NULL,
undesirable_function_linter = NULL,
function_argument_linter = NULL,
indentation_linter = NULL,
object_name_linter = NULL,
cyclocomp_linter(25L)
)
exclusions: c(
list.files("tests", recursive = TRUE, full.names = TRUE),
list.files("inst", recursive = TRUE, full.names = TRUE),
"vignettes/germany-age-stratified-nowcasting.Rmd.orig"
)
2 changes: 1 addition & 1 deletion DESCRIPTION
@@ -1,6 +1,6 @@
Package: epinowcast
Title: Flexible Hierarchical Nowcasting
Version: 0.2.0.7000
Version: 0.2.0.8000
Authors@R:
c(person(given = "Sam Abbott",
role = c("aut", "cre"),
Expand Down
4 changes: 3 additions & 1 deletion NEWS.md
@@ -1,4 +1,4 @@
# epinowcast 0.2.0.7000
# epinowcast 0.2.0.8000

This is release is in development. It is not yet ready for production use. If you notice problems please report them on the [issue tracker](https://github.com/epinowcast/epinowcast/issues).

Expand All @@ -9,6 +9,8 @@ This is release is in development. It is not yet ready for production use. If yo

## Package

- Added more non-default linters in `.lintr` configuration file. This file is used when `lintr::lint_package()` is run or in the new `lint-changed-files.yaml` GitHub Actions workflow. See #220 by @Bisaloo and reviewed by @pearsonca and @seeabs.
- Switched to the `lint-changed-files.yaml` GitHub Actions workflow instead of the regular `lint.yaml` to avoid annotations unrelated to the changes made in the PR. See #220 by @Bisaloo and reviewed by @pearsonca and @seeabs.
- Added tests for `summary.epinowcast()` and `plot.epinowcast()` methods. See #209 by @seabbs and reviewed by @pearsonca.
- Added tests for `enw_plot_obs()` where not otherwise covered by `plot.epinowcast()` tests. See #209 by @seabbs and reviewed by @pearsonca.
- Made the `.group` variable optional for all preprocessing functions using a new `add_group()` internal function. See #208 by @seabbs and reviewed by @pearsonca.
Expand Down
15 changes: 8 additions & 7 deletions R/check.R
Expand Up @@ -17,7 +17,7 @@ check_quantiles <- function(posterior, req_probs = c(0.5, 0.95, 0.2, 0.8)) {
if (sum(cols %in% paste0("q", req_probs * 100)) != length(req_probs)) {
stop(
"Following quantiles must be present (set with probs): ",
paste(req_probs, collapse = ", ")
toString(req_probs)
seabbs marked this conversation as resolved.
Show resolved Hide resolved
)
}
return(invisible(NULL))
Expand Down Expand Up @@ -88,16 +88,16 @@ check_group <- function(obs) {
#' @return NULL
#'
#' @family check
check_by <- function(obs, by = c()) {
check_by <- function(obs, by = NULL) {
if (length(by) > 0) {
if (!is.character(by)) {
stop("`by` must be a character vector")
}
if (!all(by %in% colnames(obs))) {
stop(
"`by` must be a subset of the columns in `obs`. \n",
paste0(paste(by[!(by %in% colnames(obs))], collapse = ", "),
" are not present in `obs`")
toString(by[!(by %in% colnames(obs))]),
" are not present in `obs`"
)
}
}
Expand Down Expand Up @@ -151,15 +151,16 @@ check_modules_compatible <- function(modules) {
modules[[4]]$data$model_miss &&
!modules[[6]]$data$likelihood_aggregation
) {
warning(paste0(
warning(
"Incompatible model specification: A missingness model has ",
"been specified but likelihood aggregation is specified as ",
"by snapshot. Switching to likelihood aggregation by group.",
" This has no effect on the nowcast but limits the ",
"number of threads per chain to the number of groups. To ",
"silence this warning, set the `likelihood_aggregation` ",
"argument in `enw_fit_opts` to 'groups'."
), immediate. = TRUE)
"argument in `enw_fit_opts` to 'groups'.",
immediate. = TRUE
)
}
return(invisible(NULL))
}
4 changes: 2 additions & 2 deletions R/epinowcast.R
Expand Up @@ -115,9 +115,9 @@ epinowcast <- function(data,
),
expectation = epinowcast::enw_expectation(
r = ~ 0 + (1 | day:.group),
generation_time = c(1),
generation_time = 1,
observation = ~1,
latent_reporting_delay = c(1),
latent_reporting_delay = 1,
data = data
),
missing = epinowcast::enw_missing(
Expand Down