[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Simplify conditional expressions involved in vignette evaluation #341

Open
3 of 11 tasks
IndrajeetPatil opened this issue Jan 3, 2023 · 6 comments
Open
3 of 11 tasks

Simplify conditional expressions involved in vignette evaluation #341

IndrajeetPatil opened this issue Jan 3, 2023 · 6 comments
Assignees
@bwiernik @DominiqueMakowski @IndrajeetPatil @rempsyc @strengejacke @mattansb @etiennebacher
Labels
Code Style 👩‍💻 Core Packages 📦 Discussion and planning about core packages of easystats Upkeep 🧹

Comments

@IndrajeetPatil
Copy link
Member
IndrajeetPatil commented Jan 3, 2023 •
edited by rempsyc
Loading

Preamble

To pass the "noSuggests" check, we use the recommended approach of evaluating vignettes conditional on availability of all soft/weak dependencies. But, currently, the way we do it is quite complex and difficult to discern and extend. We can simplify this significantly, as shown below.

Before

if (!requireNamespace("bayestestR", quietly = TRUE) ||
  !requireNamespace("httr", quietly = TRUE) || # needed for `insight::download_model()`
  !requireNamespace("rstanarm", quietly = TRUE) ||
  !requireNamespace("ggplot2", quietly = TRUE) ||
  !requireNamespace("brms", quietly = TRUE) ||
  !requireNamespace("logspline", quietly = TRUE) ||
  !requireNamespace("insight", quietly = TRUE)) {
  knitr::opts_chunk$set(eval = FALSE)
}

After

Note that adding or removing any of the needed deps is as simple as modifying the character vector of package names.

# `{httr}` is needed for `insight::download_model()`
pkgs <- c("bayestestR", "brms", "ggplot2", "httr", "insight", "logspline", "rstanarm")
successfully_loaded <- vapply(pkgs, requireNamespace, FUN.VALUE = logical(1L), quietly = TRUE)
can_evaluate <- all(successfully_loaded)

if (can_evaluate) {
  knitr::opts_chunk$set(eval = TRUE)
  vapply(pkgs, require, FUN.VALUE = logical(1L), quietly = TRUE, character.only = TRUE)
} else {
  knitr::opts_chunk$set(eval = FALSE)
}

Progress tracker:

  • insight
  • modelbased
  • bayestestR
  • effectsize
  • parameters
  • performance
  • correlation
  • report
  • see
  • datawizard
  • easystats
@IndrajeetPatil IndrajeetPatil added Core Packages 📦 Discussion and planning about core packages of easystats Code Style 👩‍💻 Upkeep 🧹 labels Jan 3, 2023
IndrajeetPatil added a commit to easystats/see that referenced this issue Jan 3, 2023
@IndrajeetPatil
Simplify conditional expressions involved in vignette evaluation
e55d845 
easystats/easystats#341
@IndrajeetPatil IndrajeetPatil mentioned this issue Jan 3, 2023
Merged
@IndrajeetPatil
Copy link
Member Author

For a reference, see my PR in {see} that does this simplification.

IndrajeetPatil added a commit to easystats/see that referenced this issue Jan 3, 2023
@IndrajeetPatil
Simplify conditional expressions involved in vignette evaluation (#258)
15ada9d 
* Simplify conditional expressions involved in vignette evaluation

easystats/easystats#341

* Apply automatic changes

* fix docs

* fix lints

* simplify conditionals

* Update scale_color_okabeito.Rd

Co-authored-by: IndrajeetPatil <IndrajeetPatil@users.noreply.github.com>
@mattansb
Copy link
Member
mattansb commented Jan 4, 2023

In {effectsize} I've gone a slightly different route - instead of skipping all execution if one package is missing, just the relevant chinks are skipped. So I consider {effectsize} as closed on this issue.

@IndrajeetPatil
Copy link
Member Author

@mattansb Sounds good to me!

Yes, this is relevant for the repo you are maintaining only if you are using the outlined pattern.

@IndrajeetPatil IndrajeetPatil mentioned this issue Jan 21, 2023
Closed
11 tasks
@rempsyc
Copy link
Member
rempsyc commented Jan 30, 2023 •
edited
Loading

For a given vignette, for how many external packages is it worth changing? E.g., for a single package? Two packages? report has only one or two external packages per vignette.

(seems a bit overkill for a single package, going from 3 lines to 9 lines, so I haven't changed those yet)

@rempsyc rempsyc mentioned this issue Jan 30, 2023
Merged
@IndrajeetPatil
Copy link
Member Author

For a given vignette, for how many external packages is it worth changing? E.g., for a single package? Two packages? report has only one or two external packages per vignette.

(seems a bit overkill for a single package, going from 3 lines to 9 lines, so I haven't changed those yet)

I think, for vignettes using more than one package, this makes sense.
Because if you need to use an additional package, modifying this code is as simple as extending the vector of package names.

@IndrajeetPatil IndrajeetPatil mentioned this issue Apr 3, 2023
Merged
@IndrajeetPatil
Copy link
Member Author

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bwiernik bwiernik

@DominiqueMakowski DominiqueMakowski

@IndrajeetPatil IndrajeetPatil

@rempsyc rempsyc

@strengejacke strengejacke

@mattansb mattansb

@etiennebacher etiennebacher

Labels
Code Style 👩‍💻 Core Packages 📦 Discussion and planning about core packages of easystats Upkeep 🧹
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@bwiernik @DominiqueMakowski @IndrajeetPatil @rempsyc @strengejacke @mattansb @etiennebacher