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

M-Kusumgar · 2024-01-24T19:10:08Z

I have extracted out most instances of "orderly.R" from the code and used a util get_orderly_file_names to extract out the allowed file names. I have also tried to use vector methods as much as possible so that if we want to add another supported filename such as "orderly-.R" I believe we should be able to do so by just changing that vector (and changing docs which have to have the hardcoding of file names). Let me know if that isn't the case!

Also, should I bump version number for this? And is there a NEWS.md file or anything like that? Can't find it.

codecov · 2024-01-24T19:14:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d174d41) to head (ae37530).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #122   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           40        40           
  Lines         3589      3621   +32     
=========================================
+ Hits          3589      3621   +32

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

richfitz

Overall, I think that the idea of name/name.R is good given the functionality of RStudio, but I am wondering if it would be better to remove the choice here.

So perhaps rework slightly to add a deprecation warning if we find <name>/orderly.R and at that point save into the packet runtime information the name that we found? Then later on we can just error if the user has orderly.R

Other minor comments included, but probably quite a bit of the PR would change if you agree with this suggestion. Curious on Rob's thoughts, and @plietar too.

richfitz · 2024-01-25T14:07:57Z

R/metadata.R

-        i = "You have already called this function earlier in your orderly.R"),
+      c("Only one call to 'orderly2::orderly_{name}' is allowed"),
+        i = paste("You have already called this function earlier",
+                  "in your orderly file"),


Can we do:

"in '{actual-name.R}'"),

that would be nicer if we hold that here. We should save this in the orderly-specific packet data perhaps while running?

richfitz · 2024-01-25T14:08:33Z

R/orderly.R

-##' List source reports - that is, directories within `src/` that
-##' contain a file `orderly.R`
+##' List source reports - that is, directories `src/<reportname>` that
+##' contain one of `<reportname>.R` or `orderly.R`


Is this the point where we just disallow orderly.R entirely, and only have src/<reportname>/<reportname>.R?

richfitz · 2024-01-25T14:09:22Z

R/orderly.R

@@ -21,7 +21,10 @@ 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"))]
+  file_paths <- lapply(pos, function(p) {


Suggested change

file_paths <- lapply(pos, function(p) {

file_paths <- vcapply(pos, function(p) {

map returning character vector I think?

richfitz · 2024-01-25T14:11:08Z

R/util.R

  files <- c(...)
  if (!is.null(workdir)) {
    assert_scalar_character(workdir)
    owd <- setwd(workdir) # nolint
    on.exit(setwd(owd)) # nolint
  }
-  fs::file_exists(files)
+  if (single_file) {


I think you have two different functions here masquerading as one with an option?

r-ash

This looks good, but I think if you move the filename handling stuff into orderly_read and then return the path from that function this might simplify up some of the repeated calls to get_orderly_file_names

I think also worth putting this into the orderly2 packet metadata that gets created at 208 and then using this instead of finding the filename again in custom_metadata.

Can you add to the vignettes too?

r-ash · 2024-01-25T09:24:05Z

R/cleanup.R

@@ -138,7 +139,7 @@ orderly_cleanup_status <- function(name = NULL, root = NULL, locate = TRUE) {
  nms_dependency <- unlist(lapply(info$dependency, function(x) names(x$files)))
  nms_shared_resource <- names(info$shared_resource)

-  role <- cbind(orderly = files == "orderly.R",
+  role <- cbind(orderly = vlapply(files, "%in%", get_orderly_file_names(path)),


I don't think you need the vlapply here, %in% works with vector on either side

c("on", "two") %in% c("on", "three", "four") [1] TRUE FALSE

r-ash · 2024-01-25T09:26:51Z

R/util.R

+    vlapply(files, function(f) {
+      sum(fs::file_exists(f)) == 1
+    })


You can skip the vlapply here too, fs::file_exists will take a vector

fs::file_exists(c("notafile", "DESCRIPTION")) notafile DESCRIPTION FALSE TRUE

res <- fs::file_exists(files) if (single_file) { res <- sum(res) == 1 } res

Though I think there is an argument for making this a separate function as file_exists returns same length as input files but with single_file = TRUE now returns a single value

r-ash · 2024-01-25T10:43:10Z

R/metadata.R

+      c("Only one call to 'orderly2::orderly_{name}' is allowed"),
+        i = paste("You have already called this function earlier",
+                  "in your orderly file"),


Think a small typo here

Suggested change

c("Only one call to 'orderly2::orderly_{name}' is allowed"),

i = paste("You have already called this function earlier",

"in your orderly file"),

c("Only one call to 'orderly2::orderly_{name}' is allowed",

i = paste("You have already called this function earlier",

"in your orderly file")),

r-ash · 2024-01-25T10:49:01Z

R/util_assert.R

+  which_files_exist <- which(vlapply(filenames, function(filename) {
+    file_exists(filename, workdir = workdir)
+  }))


Suggested change

which_files_exist <- which(vlapply(filenames, function(filename) {

file_exists(filename, workdir = workdir)

}))

which_files_exist <- filenames[file_exists(filename, workdir = workdir)]

I think this works? file_exists can take a vector of filenames

r-ash · 2024-01-25T10:53:09Z

R/util_assert.R

+
+  if (length(which_files_exist) != 1) {
+    cli::cli_abort(
+      c("Please create ONE of {paste(filenames, collapse = ', ')} files",


You might be able to use CLI tools here to create x or y. And then automatically go to x, y or z if we add a third. https://cli.r-lib.org/articles/pluralization.html#use-the-length-of-character-vectors

r-ash · 2024-01-25T10:58:20Z

R/util_assert.R

+        call = call)
+  }
+
+  assert_file_exists_relative(filenames[which_files_exist], workdir, name, call)


This is never going raise an error right?

r-ash · 2024-01-25T11:00:46Z

R/orderly.R

-
-  if (file.exists(file.path(dest, "orderly.R"))) {
-    cli::cli_abort("'src/{name}/orderly.R' already exists")
+  orderly_file_names <- get_orderly_file_names(dest)


Is it ok to change the default here? @richfitz might want to keep as orderly.R?

r-ash · 2024-01-25T13:59:23Z

R/run.R

+  file_names <- get_orderly_file_names(src)
+  file_name <- file_names[file_exists(file_names, workdir = src)]


I think I'd be tempted to return this from orderly_read call above. As orderly_entrypoint or source_file or something and then refer to it here.

r-ash · 2024-01-25T14:08:30Z

R/run.R

+  file_names <- get_orderly_file_names(dat$src)
+  file_name <- file_names[file_exists(file_names, workdir = dat$src)]


I wonder if worth getting this into the orderly2 metadata on the packet. See line 208 - 210 in run.R the filename is available to you there, put it into that list and then pull it out here.

r-ash · 2024-01-25T14:12:54Z

R/gitignore.R

-  dat <- orderly_read_r(file.path(root_path, "src", name, "orderly.R"))
+  path <- file.path(root_path, "src", name)
+  file_names <- get_orderly_file_names(path)
+  dat <- orderly_read_r(


Could just use orderly_read here instead?

plietar · 2024-01-25T16:00:40Z

Overall, I think that the idea of name/name.R is good given the functionality of RStudio, but I am wondering if it would be better to remove the choice here.
So perhaps rework slightly to add a deprecation warning if we find /orderly.R and at that point save into the packet runtime information the name that we found? Then later on we can just error if the user has orderly.R

I think I agree with this. There's not much of a reason to support both, other than temporary backwards compatibility. I would change all the comments and documentation to just use <name>.R already.

plietar · 2024-01-25T16:05:41Z

R/orderly.R

-  writeLines(contents, file.path(dest, "orderly.R"))
-  cli::cli_alert_success("Created 'src/{name}/orderly.R'")
+  writeLines(contents, file.path(dest, orderly_file_names[[1]]))
+  cli::cli_alert_success("Created 'src/{name}/{orderly_file_names[[1]]}'")


It's not obvious from looking at these two lines which alternative is actually implemented.
Assuming we decide to make the new filename the blessed option, I would just make this src/{name}/{name}.R.

plietar · 2024-01-25T16:24:47Z

Generally have the same comments as @r-ash. You shouldn't need to look this up so often, orderly_read already returns a named list with various attributes about the report, and this can be added to it. Otherwise looks good.

M-Kusumgar · 2024-02-08T15:39:49Z

Alright take two of this PR, I was going to use orderly context but ran into some infinite recursion in some functions and the remaining orderly context calls were throwing test errors because testthat folder is not an orderly directory (orderly context does some checks based on the working directory I think).

Either way probably best to be consistent and use the same function everywhere and do the orderly name checking there so I have used the deprecate_old_orderly_name function. This will throw the deprecation warning every 8 hours in the terminal if they have an orderly.R file.

This also includes the fix for the test that was failing due to the outpack server PR. Thanks for the help on fixing it @plietar

plietar

Looks good. Just some minor nitpicks:

orderly_name, as are called a few variables, is quite a confusing, erm, name. It could mean anything really. I think something like script_name or entrypoint_filename is much clearer.
deprecate_old_orderly_name is also confusing. While it does indeed print a depreciation note, that's kind of a side effect. The main thing it does and why we call it is to find the name of the file. locate_report_file or find_entrypoint_file or something along those lines would be better IMO.
I think orderly_read should include the file's name in the returned dictionary. The function reads a bunch of metadata about the report, and could include that name. Most users of deprecate_old_orderly_name would have just called orderly_read a few lines prior. There are a few cases where that's not the case, and these places just call deprecate_old_orderly_name directly and that is fine IMO.

R/read.R

R/gitignore.R

R/orderly.R

R/read.R

M-Kusumgar · 2024-02-21T14:12:49Z

orderly_name, as are called a few variables, is quite a confusing, erm, name. It could mean anything really. I think something like script_name or entrypoint_filename is much clearer.

Agreed, I like entrypoint_filename so have gone with that

deprecate_old_orderly_name is also confusing. While it does indeed print a depreciation note, that's kind of a side effect. The main thing it does and why we call it is to find the name of the file. locate_report_file or find_entrypoint_file or something along those lines would be better IMO.

For consistency in the naming I have gone with find_entrypoint_filename

I think orderly_read should include the file's name in the returned dictionary. The function reads a bunch of metadata about the report, and could include that name. Most users of deprecate_old_orderly_name would have just called orderly_read a few lines prior. There are a few cases where that's not the case, and these places just call deprecate_old_orderly_name directly and that is fine IMO.

I found one example in orderly_run have returned entrypoint_filename key in list from orderly_read_r and used this in there, not sure where else I would use it, was there another place you had in mind?

…n name change

R/gitignore.R

Co-authored-by: Paul Liétar <[email protected]>

richfitz

Thanks - small suggestions here

richfitz · 2024-02-21T16:10:06Z

R/metadata.R

@@ -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")),


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

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

richfitz · 2024-02-21T16:12:02Z

R/orderly.R

@@ -1,5 +1,5 @@
 ##' List source reports - that is, directories within `src/` that
-##' contain a file `orderly.R`
+##' contain a file `<reportname>.R`


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?

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...

richfitz · 2024-02-21T16:14:19Z

R/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]


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)]

yh i like this pattern!

richfitz · 2024-02-21T16:17:06Z

R/util_assert.R

+      "Please create {names[[1]]} file"
+    )
+  } else if (files_exist[[2]]) {
+    rlang::inform(


Suggested change

rlang::inform(

cli::cli_warn(

richfitz · 2024-02-21T16:17:34Z

R/util_assert.R

+      paste("Please only create {names[[1]]} file, orderly.R",
+            "has been deprecated")
+    )
+  } else if (sum(files_exist) == 0 && !suppress_errors) {


Suggested change

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

} else if (!any(files_exist) && !suppress_errors) {

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

richfitz · 2024-02-21T16:26:31Z

R/run.R

@@ -546,6 +551,10 @@ validate_orderly_directory <- function(name, root_path, call) {
    cli::cli_abort(err, call = call)
  }

+  find_entrypoint_filename(


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

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

richfitz · 2024-02-21T16:28:24Z

R/util_assert.R

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


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

yh looks much cleaner, done

richfitz · 2024-02-21T16:28:56Z

_pkgdown.yml

@@ -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


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?

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

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

richfitz · 2024-02-21T16:30:00Z

tests/testthat/examples/computed-resource/computed-resource.R

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)

created a ticket: https://mrc-ide.myjetbrains.com/youtrack/agiles/103-58/current?issue=mrc-5100

richfitz · 2024-02-21T16:32:13Z

vignettes/dependencies.Rmd

@@ -28,7 +28,7 @@ Here, we show how to practically use dependencies in a few common scenarios of i

 ## Basic use

-The primary mechanism for using dependencies is to call `orderly2::orderly_dependency()` from within an `orderly.R` script; this finds a suitable completed packet and copies files that are found from within that packet into your current report.
+The primary mechanism for using dependencies is to call `orderly2::orderly_dependency()` from within a `<reportname>.R` script; this finds a suitable completed packet and copies files that are found from within that packet into your current report.


yeah, we need a name for this!

richfitz

Looking good.

Also, should I bump version number for this? And is there a NEWS.md file or anything like that? Can't find it.

Just bump the patch version number for now

M-Kusumgar requested review from richfitz and r-ash January 24, 2024 20:33

richfitz requested changes Jan 25, 2024

View reviewed changes

r-ash requested changes Jan 25, 2024

View reviewed changes

plietar reviewed Jan 25, 2024

View reviewed changes

M-Kusumgar added 6 commits February 1, 2024 17:39

implementation for orderly.R deprecation

8fbbe08

fix tests

ff67498

fixed example

c44fed5

add env var to ci

5665d04

lint + don't skip test

6a3ee3d

remove unused var

f42780d

r-ash mentioned this pull request Feb 7, 2024

Use existing files on disk instead of pushing afresh #125

Merged

test fix2

c779006

M-Kusumgar force-pushed the mrc-4969 branch from 5ebd6e2 to c779006 Compare February 8, 2024 14:31

fix lint and codecov

4332322

M-Kusumgar requested review from plietar, r-ash and richfitz February 8, 2024 15:43

plietar reviewed Feb 12, 2024

View reviewed changes

R/read.R Outdated Show resolved Hide resolved

R/gitignore.R Outdated Show resolved Hide resolved

R/orderly.R Outdated Show resolved Hide resolved

R/read.R Outdated Show resolved Hide resolved

paul changed: orderly_name -> entrypoint_filename + cleanup + functio…

7dfbefd

…n name change

plietar reviewed Feb 21, 2024

View reviewed changes

R/gitignore.R Outdated Show resolved Hide resolved

plietar approved these changes Feb 21, 2024

View reviewed changes

Update R/gitignore.R

3d4ef51

Co-authored-by: Paul Liétar <[email protected]>

richfitz requested changes Feb 21, 2024

View reviewed changes

rich's comments

dd99536

Merge branch 'mrc-4969' of github.com:mrc-ide/orderly2 into mrc-4969

47fa26a

M-Kusumgar requested a review from richfitz February 23, 2024 12:26

richfitz requested changes Feb 29, 2024

View reviewed changes

bump patch version num

ae37530

M-Kusumgar requested a review from richfitz February 29, 2024 15:57

r-ash approved these changes Feb 29, 2024

View reviewed changes

r-ash merged commit 4a060c1 into main Feb 29, 2024
11 checks passed

plietar mentioned this pull request Mar 19, 2024

More flexibility in the naming of the orderly.R files for multiple reports. #107

Closed

	file_paths <- lapply(pos, function(p) {
	file_paths <- vcapply(pos, function(p) {

		file_names <- get_orderly_file_names(src)
		file_name <- file_names[file_exists(file_names, workdir = src)]

		file_names <- get_orderly_file_names(dat$src)
		file_name <- file_names[file_exists(file_names, workdir = dat$src)]

-##' 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`).

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

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

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

Conversation

M-Kusumgar commented Jan 24, 2024 • edited Loading

codecov bot commented Jan 24, 2024 • edited Loading

Codecov Report

richfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-ash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plietar commented Jan 25, 2024

Choose a reason for hiding this comment

plietar commented Jan 25, 2024

M-Kusumgar commented Feb 8, 2024

plietar left a comment

Choose a reason for hiding this comment

M-Kusumgar commented Feb 21, 2024

richfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richfitz left a comment

Choose a reason for hiding this comment

M-Kusumgar commented Jan 24, 2024 •

edited

Loading

codecov bot commented Jan 24, 2024 •

edited

Loading