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

Mrc 4969 Allow <reportname>.R or orderly.R as orderly file #122

Merged
merged 13 commits into from
Feb 29, 2024
5 changes: 3 additions & 2 deletions R/cleanup.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
##' # We simulate running a packet interactively by using 'source';
##' # you might have run this line-by-line, or with the "Source"
##' # button in Rstudio.
##' source(file.path(path, "src/data/orderly.R"), chdir = TRUE)
##' source(file.path(path, "src/data/data.R"), chdir = TRUE)
##'
##' # Having run this, the output of the report is present in the
##' # source directory:
Expand Down Expand Up @@ -137,8 +137,9 @@ orderly_cleanup_status <- function(name = NULL, root = NULL, locate = TRUE) {
nms_artefact <- unlist(lapply(info$artefacts, "[[", "files"))
nms_dependency <- unlist(lapply(info$dependency, function(x) names(x$files)))
nms_shared_resource <- names(info$shared_resource)
entrypoint_filename <- find_entrypoint_filename(path, basename(path))

role <- cbind(orderly = files == "orderly.R",
role <- cbind(orderly = files == entrypoint_filename,
resource = matches_path(files, nms_resource),
shared_resource = matches_path(files, nms_shared_resource),
dependency = matches_path(files, nms_dependency, FALSE),
Expand Down
3 changes: 2 additions & 1 deletion R/gitignore.R
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ gitignore_content_root <- function(root_path) {


gitignore_content_src <- function(name, root_path) {
dat <- orderly_read_r(file.path(root_path, "src", name, "orderly.R"))
src <- file.path(root_path, "src", name)
M-Kusumgar marked this conversation as resolved.
Show resolved Hide resolved
dat <- orderly_read(file.path(root_path, "src", name))

ignore_deps <- unlist(lapply(dat$dependency, function(x) names(x$files)))
ignore_artefacts <- unlist(lapply(dat$artefacts, "[[", "files"))
Expand Down
5 changes: 3 additions & 2 deletions R/metadata.R
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static_orderly_strict_mode <- function(args) {
##' @section Behaviour in interactive sessions:
##'
##' When running interactively (i.e., via `source()` or running an
##' `orderly.R` session by copy/paste or in Rstudio), the
##' `<reportname>.R` session by copy/paste or in Rstudio), the
##' `orderly_parameters()` function has different behaviour.
##'
##' First, we look in the current environment (most likely the global
Expand Down Expand Up @@ -454,7 +454,8 @@ prevent_multiple_calls <- function(packet, name, call) {
if (!is.null(packet$orderly2[[name]])) {
cli::cli_abort(
c("Only one call to 'orderly2::orderly_{name}' is allowed",
i = "You have already called this function earlier in your orderly.R"),
i = paste("You have already called this function earlier",
"in your <reportname>.R")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass the filename down here to prevent_multiple_calls and use it in the error message, it'll be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, just did it using the packet info we have, thought that was simpler

call = call)
}
}
30 changes: 20 additions & 10 deletions R/orderly.R
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
##' List source reports - that is, directories within `src/` that
##' contain a file `orderly.R`
##' contain a file `<reportname>.R`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be reworded or explained better

Suggested change
##' contain a file `<reportname>.R`
##' look suitable for running with orderly; these will be directories
##' that contain a `.R` file with the same name as the directory
##' (e.g., `src/data/data.R` corresponds to `data`).

More of a mouthful, but perhaps clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yh definitely clearer, ive altered it slightly, have said it will list dirs that contain an entrypoint file - a .R file with the same name...

##'
##' @title List source reports
##'
Expand All @@ -21,7 +21,12 @@ orderly_list_src <- function(root = NULL, locate = TRUE) {
return(character())
}
pos <- fs::dir_ls(file.path(root_path, "src"), type = "directory")
basename(pos)[file_exists(file.path(pos, "orderly.R"))]
files_exist <- vlapply(pos, function(path) {
entrypoint_filename <- find_entrypoint_filename(path, basename(path),
suppress_errors = TRUE)
!is.null(entrypoint_filename)
})
basename(pos)[files_exist]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the find_entrypoint_filename returned NA_character_ you could simplify slightly

entrypoint <- vcapply(pos, function(path) find_entrypoint_filename(path, basename(path), suppress_errors = TRUE)
basename(pos)[!is.na(entrypoint)]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yh i like this pattern!

}


Expand All @@ -36,8 +41,8 @@ orderly_list_src <- function(root = NULL, locate = TRUE) {
##' suppresses any default content. We may support customisable
##' templates in future - let us know if this would be useful.
##'
##' @param force Create an `orderly.R` file within an existing
##' directory `src/<name>`; this may be useful if you have already
##' @param force Create a `<reportname>.R` file within an existing
##' directory `src/<reportname>`; this may be useful if you have already
##' created the directory and some files first but want help
##' creating the orderly file.
##'
Expand All @@ -49,9 +54,13 @@ orderly_new <- function(name, template = NULL, force = FALSE,
root = NULL, locate = TRUE) {
root <- root_open(root, locate, require_orderly = TRUE, call = environment())
dest <- file.path(root$path, "src", name)
existing_entrypoint_filename <- find_entrypoint_filename(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite correct, because you'll mishandle the case where we have src/foo/foo.R and src/foo/orderly.R so we return NULL and think we can continue and then overwrite the existing file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be able to verify this with your two-orderly-files test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep so ive slightly changed the suppress_errors arg and split it into suppress_zero_files and suppress_multiple_files and added a test with my two-orderly-files to make sure it error, first time ive done with TTD :O

dest, name, suppress_errors = TRUE
)
new_report_filename <- sprintf("%s.R", name)

if (file.exists(file.path(dest, "orderly.R"))) {
cli::cli_abort("'src/{name}/orderly.R' already exists")
if (!is.null(existing_entrypoint_filename)) {
cli::cli_abort("'src/{name}/{existing_entrypoint_filename}' already exists")
}
if (file.exists(dest) && !fs::is_dir(dest)) {
cli::cli_abort(
Expand All @@ -61,8 +70,9 @@ orderly_new <- function(name, template = NULL, force = FALSE,
if (length(dir(dest, all.files = TRUE, no.. = TRUE)) > 0 && !force) {
cli::cli_abort(
c("'src/{name}/' already exists and contains files",
i = paste("If you want to add an orderly.R to this directory,",
"rerun {.code orderly_new()} with {.code force = TRUE}")))
i = paste("If you want to add a {new_report_filename} to this",
"directory, rerun {.code orderly_new()} with",
"{.code force = TRUE}")))
}

if (is.null(template)) {
Expand All @@ -74,6 +84,6 @@ orderly_new <- function(name, template = NULL, force = FALSE,
}

fs::dir_create(dest)
writeLines(contents, file.path(dest, "orderly.R"))
cli::cli_alert_success("Created 'src/{name}/orderly.R'")
writeLines(contents, file.path(dest, new_report_filename))
cli::cli_alert_success("Created 'src/{name}/{new_report_filename}'")
}
2 changes: 1 addition & 1 deletion R/plugin.R
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
##' Create an orderly plugin. A plugin is typically defined by a
##' package and is used to extend orderly by enabling new
##' functionality, declared in `orderly_config.yml` and `orderly.R`
##' functionality, declared in `orderly_config.yml` and `<reportname>.R`
##' and affecting the running of reports primarily by creating new
##' objects in the report environment. This system is discussed in
##' more detail in `vignette("plugins")`, but will be expanded (likely
Expand Down
14 changes: 7 additions & 7 deletions R/read.R
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
orderly_read <- function(path, call = NULL) {
assert_file_exists_relative("orderly.R", name = "Orderly file",
workdir = path, call = call)
orderly_read_r(file.path(path, "orderly.R"))
entrypoint_filename <- find_entrypoint_filename(path, basename(path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost every call here has this pattern. Consider refactoring so that you can do

find_entrypoint_filename(base)

(i.e., move the basename within find_entrypoint_filename) which will make this a little less noisy I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

orderly_read_r(file.path(path, entrypoint_filename), entrypoint_filename)
}


orderly_read_r <- function(path) {
orderly_read_r <- function(path, entrypoint_filename) {
exprs <- parse(file = path)

inputs <- list()
Expand Down Expand Up @@ -51,7 +50,7 @@ orderly_read_r <- function(path) {
## Rename to make things easier below:
names(dat) <- sub("^orderly_", "", names(dat))

ret <- list()
ret <- list(entrypoint_filename = entrypoint_filename)
if (length(dat$strict_mode) > 0) {
ret$strict <- dat$strict_mode[[1]]
} else {
Expand All @@ -72,11 +71,12 @@ orderly_read_r <- function(path) {
## TODO: probably some santisiation required here:
##
## * what do we do with directories here?
## * discourage people from listing orderly.R
## * discourage people from listing <reportname>.R
## * discourage duplicates

if (length(dat$resource) > 0) {
ret$resources <- setdiff(unique(unlist(dat$resource, TRUE, FALSE)),
"orderly.R")
entrypoint_filename)
}
if (length(dat$artefact) > 0) {
ret$artefacts <- dat$artefact
Expand Down
53 changes: 31 additions & 22 deletions R/run.R
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,

src <- file.path(root_src, "src", name)
dat <- orderly_read(src, environment())
entrypoint_filename <- dat$entrypoint_filename
parameters <- check_parameters(parameters, dat$parameters, environment())
orderly_validate(dat, src)

Expand All @@ -194,14 +195,14 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,

if (dat$strict$enabled) {
inputs_info <- NULL
fs::file_copy(file.path(src, "orderly.R"), path)
fs::file_copy(file.path(src, entrypoint_filename), path)
} else {
inputs_info <- copy_resources_implicit(src, path, dat$resources,
dat$artefacts)
}
p <- outpack_packet_start(path, name, parameters = parameters,
id = id, root = root)
outpack_packet_file_mark(p, "orderly.R", "immutable")
outpack_packet_file_mark(p, entrypoint_filename, "immutable")
p$orderly2 <- list(config = root$config$orderly, envir = envir, src = src,
root = root_src, strict = dat$strict,
inputs_info = inputs_info, search_options = search_options)
Expand All @@ -216,7 +217,7 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,
local <- new.env(parent = emptyenv())
local$warnings <- collector()
res <- rlang::try_fetch(
withr::with_dir(path, source_echo("orderly.R", envir, echo)),
withr::with_dir(path, source_echo(entrypoint_filename, envir, echo)),
warning = function(e) {
local$warnings$add(e)
rlang::zap()
Expand All @@ -233,9 +234,9 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,
success <- is.null(local$error) && info_end$success

if (success) {
cli::cli_alert_success("Finished running {.file orderly.R}")
cli::cli_alert_success("Finished running {.file {entrypoint_filename}}")
} else {
cli::cli_alert_danger("Error running {.file orderly.R}")
cli::cli_alert_danger("Error running {.file {entrypoint_filename}}")
}

if (length(local$warnings$get()) > 0) {
Expand Down Expand Up @@ -265,9 +266,10 @@ orderly_run <- function(name, parameters = NULL, envir = NULL, echo = TRUE,


custom_metadata <- function(dat) {
entrypoint_filename <- find_entrypoint_filename(dat$src, basename(dat$src))
shared <- dat$shared_resources %||% list()
role <- data_frame(
path = c("orderly.R", dat$resources, shared$here),
path = c(entrypoint_filename, dat$resources, shared$here),
role = c("orderly",
rep_along("resource", dat$resources),
rep_along("shared", shared$here)))
Expand Down Expand Up @@ -517,24 +519,27 @@ orderly_packet_add_metadata <- function(p) {

validate_orderly_directory <- function(name, root_path, call) {
assert_scalar_character(name, call = call)

re <- "^(./)*(src/)?(.+?)/?$"
name <- sub(re, "\\3", name)
if (!file_exists(file.path(root_path, "src", name, "orderly.R"))) {
src <- file.path(root_path, "src", name)
err <- sprintf("Did not find orderly report '%s'", name)
if (!file_exists(src)) {
detail <- sprintf("The path 'src/%s' does not exist", name)
} else if (is_directory(src)) {
detail <- sprintf(
"The path 'src/%s' exists but does not contain 'orderly.R'",
name)
} else {
detail <- sprintf(
"The path 'src/%s' exists but is not a directory",
name)
}
err <- c(err, x = detail)
src <- file.path(root_path, "src", name)

is_error <- FALSE
if (!file_exists(src)) {
is_error <- TRUE
detail <- sprintf("The path 'src/%s' does not exist", name)
} else if (!is_directory(src)) {
is_error <- TRUE
detail <- sprintf(
"The path 'src/%s' exists but is not a directory",
name
)
}

if (is_error) {
err <- c(
sprintf("Did not find orderly report '%s'", name),
x = detail
)
near <- near_match(name, orderly_list_src(root_path, FALSE))
if (length(near) > 0) {
hint <- sprintf("Did you mean %s",
Expand All @@ -546,6 +551,10 @@ validate_orderly_directory <- function(name, root_path, call) {
cli::cli_abort(err, call = call)
}

find_entrypoint_filename(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just being called for the warning/error? A comment perhaps? Will this be removed after we remove the deprecation warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep added a comment, this is just for the deprecation warning

file.path(root_path, "src", name), name
)

name
}

Expand Down
31 changes: 31 additions & 0 deletions R/util_assert.R
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,37 @@ assert_file_exists <- function(files, name = "File", call = NULL, arg = NULL) {
}


find_entrypoint_filename <- function(src, reportname,
suppress_errors = FALSE) {
names <- c(sprintf("%s.R", reportname), "orderly.R")
files_exist <- file.exists(file.path(src, names))
if (sum(files_exist) > 1 && !suppress_errors) {
cli::cli_abort(
paste("Please only create {names[[1]]} file, orderly.R",
"has been deprecated")
)
} else if (sum(files_exist) == 0 && !suppress_errors) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
} else if (sum(files_exist) == 0 && !suppress_errors) {
} else if (!any(files_exist) && !suppress_errors) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since I am implementing your other suggestion about assigning n_found <- sum(files_exist) i think i should stick to comparing value of n_found

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our normal style here is not to have an else clause after an if that throws. So

if (whatever) {
  throw
}
otherwise

not

if (whatever) {
  throw
} else {
  otherwise
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep done!

cli::cli_abort(
"Please create {names[[1]]} file"
)
} else if (files_exist[[2]]) {
rlang::inform(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
rlang::inform(
cli::cli_warn(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

paste("Naming convention orderly.R will be deprecated",
"soon. Please change orderly file name to",
"<reportname>.R"),
.frequency = "regularly",
.frequency_id = "deprecate_orderly_file_name"
)
}

if (suppress_errors && sum(files_exist) != 1) {
NULL
} else {
names[files_exist]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (suppress_errors && sum(files_exist) != 1) {
NULL
} else {
names[files_exist]
}
if (sum(files_exist) == 1) names[files_exist] else NA_character_
  • no need to test suppress_errors
  • see above for suggestion to replace return type with missing string rather than NULL
  • can do it on one line, which I find sometimes clearer

Also you sum files_exist several times, here could pull that into n_found <- sum(files_exist) if you want

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yh looks much cleaner, done

}


assert_file_exists_relative <- function(files, workdir, name, call = NULL) {
assert_relative_path(files, name, workdir, call)

Expand Down
2 changes: 1 addition & 1 deletion _pkgdown.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ reference:
- orderly_list_src
- title: From within a running report
desc: >-
These are the functions that get called from your `orderly.R` file
These are the functions that get called from your `<reportname>.R` file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a nicer name for this. Perhaps calling it your "orderly file" or "entrypoint file" everywhere would make it nicer to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yh i think ill do entrypoint file? will be consistent with our variable naming!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although entrypoint file is a bit difficult to understand out of context, maybe for docs we go for orderly file? might be clearer for the users

contents:
- orderly_strict_mode
- orderly_parameters
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion man/orderly_cleanup.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion man/orderly_list_src.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions man/orderly_new.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion man/orderly_parameters.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions man/orderly_plugin_register.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another PR we're going to need DYM support here for people getting things like underscores and hyphens confused (they do this quite a lot already)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File renamed without changes.
3 changes: 3 additions & 0 deletions tests/testthat/examples/deprecated-orderly-name/orderly.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
orderly2::orderly_artefact("Some data", "data.rds")
d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10))
saveRDS(d, "data.rds")
3 changes: 3 additions & 0 deletions tests/testthat/examples/two-orderly-files/orderly.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
orderly2::orderly_artefact("Some data", "data.rds")
d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10))
saveRDS(d, "data.rds")
3 changes: 3 additions & 0 deletions tests/testthat/examples/two-orderly-files/two-orderly-files.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
orderly2::orderly_artefact("Some data", "data.rds")
d <- data.frame(a = 1:10, x = runif(10), y = 1:10 + runif(10))
saveRDS(d, "data.rds")
Loading
Loading