HARPgroup / HARParchive

This repo houses HARP code development items, resources, and intermediate work products.
1 stars 0 forks source link

Remove global declarations of variable names, and replace variables whose names conflict with existing package functions #1268

Open rburghol opened 3 weeks ago

rburghol commented 3 weeks ago

There were several instances where assign commands were used to overwrite names in the .GlobalEnv, which is always indicative of a problem in code, so we need to eliminate them regardless, but beyond poor coding form, it is often quite risky. Example, there were instances where we were using package function names as local variable names, and to overcome that it appears that .GlobalEnv assignments were used to set names, therefore over-writing those functions -- something that has knock-on effects if we later try to use those names (and since they were pretty important function names, it is quite likely we would encounter this).

Function name conflicts: Primarily these are what I changed:

General .GlobalEnv declarations eliminated because they are indicative of insufficient argument handling:

There is a PR here that goes a long way towards doing this: #1267 However, it is not complete since the map renders up until the point where the bubbles are added, then errors prevent the rendering of the map.

megpritch commented 3 weeks ago

@rburghol Apologies, I hadn't realized that those names already existed as functions in the environment. I only meant to use the assign() function as a way to output multiple results from the same function, as well as make it easier to test individual modules while debugging fn_mapgen errors, but I will avoid this going forward.

There are instances in other scripts and functions where we use assign()--should we not be doing this at all, or is it only problematic when we specify envir=.GlobalEnv?