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

fix prepExtendedComment called from package::fun #229

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix prepExtendedComment called from package::fun #229

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0eec37b
fix prepExtendedComment called from package::fun
pascal-sauer Nov 11, 2024
77483e5
fix
pascal-sauer Nov 11, 2024
7b1ebc5
only support ::, not :::
pascal-sauer Nov 11, 2024
4fbb642
Revert "only support ::, not :::"
pascal-sauer Nov 12, 2024
047270e
use functionCallString in toolstartmessage and prepExtendedComment
pascal-sauer Nov 12, 2024
296b9a9
test comment origin
pascal-sauer Nov 12, 2024
31fc6c1
callstring arg for toolstartmessage
pascal-sauer Nov 12, 2024
83bda51
dev version number
pascal-sauer Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .buildlibrary
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
ValidationKey: '63144016'
ValidationKey: '63179814'
AcceptedWarnings:
- 'Warning: package ''.*'' was built under R version'
- 'Warning: namespace ''.*'' is not available and has been replaced'
Expand Down
4 changes: 2 additions & 2 deletions CITATION.cff
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ cff-version: 1.2.0
message: If you use this software, please cite it using the metadata from this file.
type: software
title: 'madrat: May All Data be Reproducible and Transparent (MADRaT) *'
version: 3.15.2
date-released: '2024-11-06'
version: 3.15.3
date-released: '2024-11-11'
abstract: Provides a framework which should improve reproducibility and transparency
in data processing. It provides functionality such as automatic meta data creation
and management, rudimentary quality management, data caching, work-flow management
Expand Down
4 changes: 2 additions & 2 deletions DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
Type: Package
Package: madrat
Title: May All Data be Reproducible and Transparent (MADRaT) *
Version: 3.15.2
Date: 2024-11-06
Version: 3.15.3
Date: 2024-11-11
Authors@R: c(
person("Jan Philipp", "Dietrich", , "[email protected]", role = c("aut", "cre"),
comment = c(affiliation = "Potsdam Institute for Climate Impact Research", ORCID = "0000-0002-4309-6431")),
Expand Down
1 change: 1 addition & 0 deletions NAMESPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Generated by roxygen2: do not edit by hand

export(.testPrepExtendedComment)
export(addMapping)
export(cacheCleanup)
export(cacheCopy)
Expand Down
26 changes: 22 additions & 4 deletions R/prepExtendedComment.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,24 @@
#' @author Jan Philipp Dietrich
#' @examples
#' test <- function(a = 1) {
#' return(madrat:::prepExtendedComment(list(unit = "m", description = "example", package = "blub")))
#' }
#' test(a = 42)
#' return(madrat:::prepExtendedComment(list(unit = "m", description = "example", package = "blub")))
#' }
#' test(a = 42)
#'
prepExtendedComment <- function(x, type = "#undefined", warn = TRUE, n = 1) {

# extract function call information for the parent call defined by n
cl <- sys.call(-n)
f <- get(as.character(cl[[1]]), mode = "function", sys.frame(-n - 1))
functionCall <- as.character(cl[[1]])

# if readSource is called as madrat::readSource functionName will
# be in this unintuitive order c("::", "madrat", "readSource")
if (length(functionCall) == 3 && functionCall[[1]] == "::") {
f <- get(functionCall[[3]], envir = loadNamespace(functionCall[[2]]), mode = "function", sys.frame(-n - 1))
} else {
f <- get(functionCall, mode = "function", sys.frame(-n - 1))
}

Copy link
Member

Choose a reason for hiding this comment

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

A bit unsafe to look for the function in some environment when functionCall[[2]] explicitly states it. But this is an internal function, so OK.
But why not

functionCall <- tail(as.character(cl[[1]]), 1)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit unsafe to look for the function in some environment when functionCall[[2]] explicitly states it

I assume you refer to the else branch? This is the behavior we had previously which I did not see a reason to change.

functionCall <- tail(as.character(cl[[1]]), 1)

I assume you suggest to skip the if else and use this instead to then call get(functionCall, mode = "function", sys.frame(-n - 1))? This won't work in a scenario where madrat::readSource is called, unless madrat is also attached (in which case readSource likely won't be called with the madrat:: prefix), because get would only look on the search path.

Copy link
Member

Choose a reason for hiding this comment

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

The pos argument can specify the environment in which to look for the object in any of several ways: as a positive integer (the position in the search list); as the character string name of an element in the search list; or as an environment (including using sys.frame to access the currently active function calls). The default of -1 indicates the current environment of the call to get. The envir argument is an alternative way to specify an environment.

—https://rdrr.io/r/base/get.html

The fully qualified call is

f <- get(functionCall[[3]], 
         envir = loadNamespace(functionCall[[2]]),
         mode = "function",
         pos = sys.frame(-n - 1))

Now, which argument is used, envir or pos (as they point to different things)?

Spoiler: It is envir.

Since prepExtendedComment() is not exported, people calling it without attaching madrat are wholly responsible for their own pain ;).

But a safe formulation would be

f <- get(x = tail(functionCall, 1),
         envir = ifelse(1 == length(functionCall),
                        sys.frame(-n - 1),                # function() call
                        getNamespace(functionCall[[2]])), # pkg::function() call
         mode = "function")

Copy link
Member

Choose a reason for hiding this comment

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

(I assumed – wrongly – that pos would take precedence over envir and that get(…, sys.frame(-n - 1), inherits = TRUE) would just climb up the search path until some matching function is found.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found a way to get rid of this call stack inspection altogether

cl <- match.call(definition = f, call = cl)

if (isTRUE(warn)) {
Expand Down Expand Up @@ -50,3 +59,12 @@ prepExtendedComment <- function(x, type = "#undefined", warn = TRUE, n = 1) {
date)
return(extendedComment)
}

#' .testPrepExtendedComment
#'
#' This function exists only for testing purposes.
#'
#' @export
.testPrepExtendedComment <- function() {
return(prepExtendedComment(list(unit = "m", description = "example", package = "blub")))
}
6 changes: 3 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# May All Data be Reproducible and Transparent (MADRaT) *

R package **madrat**, version **3.15.2**
R package **madrat**, version **3.15.3**

[![CRAN status](https://www.r-pkg.org/badges/version/madrat)](https://cran.r-project.org/package=madrat) [![DOI](https://zenodo.org/badge/DOI/10.5281/zenodo.1115490.svg)](https://doi.org/10.5281/zenodo.1115490) [![R build status](https://github.com/pik-piam/madrat/workflows/check/badge.svg)](https://github.com/pik-piam/madrat/actions) [![codecov](https://codecov.io/gh/pik-piam/madrat/branch/master/graph/badge.svg)](https://app.codecov.io/gh/pik-piam/madrat) [![r-universe](https://pik-piam.r-universe.dev/badges/madrat)](https://pik-piam.r-universe.dev/builds)

Expand Down Expand Up @@ -55,7 +55,7 @@ In case of questions / problems please contact Jan Philipp Dietrich <dietrich@pi

To cite package **madrat** in publications use:

Dietrich J, Baumstark L, Wirth S, Giannousakis A, Rodrigues R, Bodirsky B, Leip D, Kreidenweis U, Klein D, Sauer P (2024). _madrat: May All Data be Reproducible and Transparent (MADRaT)_. doi:10.5281/zenodo.1115490 <https://doi.org/10.5281/zenodo.1115490>, R package version 3.15.2, <https://github.com/pik-piam/madrat>.
Dietrich J, Baumstark L, Wirth S, Giannousakis A, Rodrigues R, Bodirsky B, Leip D, Kreidenweis U, Klein D, Sauer P (2024). _madrat: May All Data be Reproducible and Transparent (MADRaT)_. doi:10.5281/zenodo.1115490 <https://doi.org/10.5281/zenodo.1115490>, R package version 3.15.3, <https://github.com/pik-piam/madrat>.

A BibTeX entry for LaTeX users is

Expand All @@ -64,7 +64,7 @@ A BibTeX entry for LaTeX users is
title = {madrat: May All Data be Reproducible and Transparent (MADRaT)},
author = {Jan Philipp Dietrich and Lavinia Baumstark and Stephen Wirth and Anastasis Giannousakis and Renato Rodrigues and Benjamin Leon Bodirsky and Debbora Leip and Ulrich Kreidenweis and David Klein and Pascal Sauer},
year = {2024},
note = {R package version 3.15.2},
note = {R package version 3.15.3},
url = {https://github.com/pik-piam/madrat},
doi = {10.5281/zenodo.1115490},
}
Expand Down
11 changes: 11 additions & 0 deletions man/dot-testPrepExtendedComment.Rd
View file
Open in desktop

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

6 changes: 3 additions & 3 deletions man/prepExtendedComment.Rd
View file
Open in desktop

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

6 changes: 6 additions & 0 deletions tests/testthat/test-prepExtendedComment.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
test_that("prepExtendedComment works in functions called via package::fun", {
# need to run in separate R session to make sure madrat is not attached / loaded via load_all
expect_identical(callr::r(function() madrat::.testPrepExtendedComment()[1:2]),
c(" description: example",
" unit: m"))
})
Loading