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.

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

getMadratGraph() returns false negatives on valid code #92

Open
0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q opened this issue Dec 2, 2021 · 5 comments
Assignees
Labels

Comments

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member

This regular expression

pattern <- "(readSource|calcOutput)\\( *([^=\"',]*=|) *(\"|')?([^\"',]*)[\"']?"

fails to see that calcOutput(GDP) is valid R code (as long as GDP is defined, which it does not check).

> stringi::stri_match_all_regex("calcOutput(GDP)", "(readSource|calcOutput)\\( *([^=\"',]*=|) *(\"|')?([^\"',]*)[\"']?")
[[1]]
     [,1]              [,2]         [,3] [,4] [,5]  
[1,] "calcOutput(GDP)" "calcOutput" ""   NA   "GDP)"

This leads to false negative warnings for https://github.com/pik-piam/mrcommons/blob/0041fd9d6e6a7e564f7ea10804e31523dec8d792/R/calcGDPppp.R#L20

  warning("calcGDPppp() is deprecated. Returning default 'calcOutput(GDP)' output instead. Please use mrdrivers::calcGDP() directly.")

which is not code but a string too boot.

~ WARNING: Following functions contain read or calc statements which could not be identified: 
   
~ calcGDPppp
  Please adress the type explicitly in the call to allow
~  for proper detection, e.g. readSource("MySource")
@johanneskoch94
Copy link
Member

Ahh thanks for looking into this Michaja. The quick fix (on the calcGDPppp side) would be then to simply put GDP between quotes in the warning message right?

@tscheypidi
Copy link
Member

No, because then would getMadratGraph detect calcGDPas a dependency ofcalcGDPpppwhich would have implications for the caching. Woraround would be to writemrdrivers::calcGDPinstead ofcalcOutput(GDP)`.

For a better solution in madrat: I am not sure how to properly detect that in this case it is actually not code but just text

@johanneskoch94
Copy link
Member

Ah ok, thanks for the explanation. I will rewrite the warning message.

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented Dec 2, 2021

You can also just hide the calcOutput call.

-  warning("calcGDPppp() is deprecated. Returning default 'calcOutput(GDP)' output instead. Please use mrdriv>
+  warning("calcGDPppp() is deprecated. Returning default 'calc", "Output(GDP)' output instead. Please use mrdrivers::calcGDP() directly.")

@0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q
Copy link
Member Author

For a better solution in madrat: I am not sure how to properly detect that in this case it is actually not code but just text

Bad news: you are trying to do the impossible – parsing code using a stateless machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants