Closed robkooper closed 1 year ago
One way of checking for these is to inspect the "checking R code for possible problems" section of package check results for a NOTE to the effect of no visible binding for global variable ‘settings’
. Here's what I get today on 2020-05-11 [edited to replace outdated list with current results from cached R check logs]:
$ grep --include="Rcheck_reference.log" -R 'global variable.*settings' .
./modules/benchmark/tests/Rcheck_reference.log:check_BRR: no visible binding for global variable ‘settings’
./modules/assim.sequential/tests/Rcheck_reference.log:EnKF.MultiSite: no visible binding for global variable ‘settings’
./modules/assim.sequential/tests/Rcheck_reference.log:GEF.MultiSite: no visible binding for global variable ‘settings’
./modules/assim.sequential/tests/Rcheck_reference.log:sda.particle: no visible binding for global variable ‘settings’
./modules/data.land/tests/Rcheck_reference.log:diametergrow: no visible binding for global variable ‘settings’
./base/utils/tests/Rcheck_reference.log:convert.input: no visible binding for global variable ‘settings’
./base/db/tests/Rcheck_reference.log:query.data: no visible binding for global variable ‘settings’
./base/db/tests/Rcheck_reference.log:query.priors: no visible binding for global variable ‘settings’
./base/db/tests/Rcheck_reference.log:query.traits: no visible binding for global variable ‘settings’
Note that this only works in package code, i.e. it won't catch global settings objects in workflow.R
or in inst/
scripts.
This issue is stale because it has been open 365 days with no activity.
Unstaling: Still not finished! But we did make progress on it this year.
This issue is stale because it has been open 365 days with no activity.
Is this issue up for grabs? @infotroph @moki1202 @robkooper
@Its-Maniaco I am not sure if this issue is active or not. Let's wait to hear from @robkooper and @infotroph.
@Its-Maniaco yes, go for it!
@Its-Maniaco It is up for grabs! But because it's been open so long, none of us have a clear idea how much work remains on it. A good first step would be to find out how many places in the repository still use settings
as a global.
I can think of a few ways one might do that (possibly by running a linter on all the code files in bulk?), but any way that finds them is fine.
@infotroph @moki1202 and I had a chat about this. We searched globally for 'no visible binding for global variable ‘settings’' , and found something common in the results returned.
Localization.FUN <- settings$state.data.assimilation$Localization.FUN # localization function
scalef <- settings$state.data.assimilation$scalef %>% as.numeric() # scale factor for localization
var.names <- sapply(settings$state.data.assimilation$state.variable, '[[', "variable.name")
Here, settings is being read as global variable. Can the fix here be to add .data$
prefix?
@infotroph @mdietze I am still not sure if adding .data$
is the right way to go here. Some context here would be very helpful. Another question is how is the settings
variable here, declared as a global variable? Also adding to all this, we can't figure out how to search for all the places that use settings
as a global variable.
I agree with @moki1202 that .data$
isn't the solution because the problem is that settings
is global, not that it's undefined within a tidyverse call. I think that within functions we need to make sure that settings
is explicitly being passed as a function argument, and that any changes to those functions get propagated to where those functions are called
@mdietze How should I proceed then?
Any function that assumes settings
as a global variable should have settings
as an argument and every usage of those functions needs to be update to add that argument
I suggest removing the "good first issue" tag or breaking this issue up package-wise. As it is, it's probably too big and not well enough defined to be a good first issue.
Next Thursday this issue will be ten years old! Maybe we can finish it by then?
Amazing. You definitely deserve a prize!
Many places in the code assume settings as a global variable, this should be removed redmine-1636