Add warning for unset 'where' argument in toolGetMapping function #180
Conversation
whitehacker
requested review from
tscheypidi,
LaviniaBaumstark,
fbenke-pik and
pfuehrlich-pik
tests/testthat/test-toolGetMapping.R
Outdated
| } | ||
| globalassign("readTest") | ||
| expect_warning(readSource("Test", convert = "onlycorrect"), | ||
| "No correct function for Test could be found. Set convert to FALSE.") |
There was a problem hiding this comment.
This test is not testing the warning you added. I think passing convert = FALSE or not passing convert at all might already be enough to trigger your warning.
There was a problem hiding this comment.
I tried both options, but the tests failed.
There was a problem hiding this comment.
In addition to Pascal's comment, this test is not checking for the right warning.
Co-authored-by: Pascal Führlich <82826417+pfuehrlich-pik@users.noreply.github.com>
| @@ -31,6 +31,16 @@ toolGetMapping <- function(name, type = NULL, where = NULL, | |||
| returnPathOnly = FALSE, activecalc = NULL) { | |||
|
|
|||
| setWrapperInactive("wrapperChecks") | |||
| if (isWrapperActive("wrapperChecks")) { | |||
There was a problem hiding this comment.
Why are you testing this condition? As this wrapper is being activated in the line before, your new code portion is never executed.
There was a problem hiding this comment.
Once you remove this line, a lot of tests fail.
The first reason I can see are iinstances where the tests call madrat functions like madrat::regionscode which currently call toolGetMapping without a where argument and therefore now produce the new warning. Here, I am not sure what the right approach is: either add the where argument to madrat functions or enhance the check to not throw the warning when called from within madrat functions. @pfuehrlich-pik what do you think?
There might be other reasons why the test fail, but we can look into them once this reason has been removed.
There was a problem hiding this comment.
Passing this question on to @tscheypidi
There was a problem hiding this comment.
the isWrapperActive should exactly achieve that the checks are only active outside of madrat functions. What happens if the checks are being performed before the wrapperChecks-wrapper is being deactived?
|
Btw, you have two WIP commits. Please merge the three commits into one for a clean git history once everything else has been solved. (I assume, Pascal and Jan would appreciate it). |
I'm fine with WIP commits, I would rather not mess with the history. Clean git history is not really a priority for madrat I believe. |
pfuehrlich-pik
force-pushed
the
master
branch
from
898567d
to
4a86b6d
Compare
As it was discussed here, the changes are made to display a warning to alert users when the
whereargument is not set intoolGetMapping()function.