Remove settings as a global variable

[robkooper]

Many places in the code assume settings as a global variable, this should be removed redmine-1636

[robkooper]

Fix https://github.com/PecanProject/pecan/blob/master/utils/R/convert.input.R#L18

[infotroph]

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.

[github-actions[bot]]

This issue is stale because it has been open 365 days with no activity.

[infotroph]

Unstaling: Still not finished! But we did make progress on it this year.

[github-actions[bot]]

This issue is stale because it has been open 365 days with no activity.

[Its-Maniaco]

Is this issue up for grabs? @infotroph @moki1202 @robkooper

[moki1202]

@Its-Maniaco I am not sure if this issue is active or not. Let's wait to hear from @robkooper and @infotroph.

[mdietze]

@Its-Maniaco yes, go for it!

[infotroph]

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

[Its-Maniaco]

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

[moki1202]

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

[mdietze]

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

[Its-Maniaco]

@mdietze How should I proceed then?

[mdietze]

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

[Aariq]

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.

[infotroph]

Next Thursday this issue will be ten years old! Maybe we can finish it by then?

[dlebauer]

Amazing. You definitely deserve a prize!