pik-piam / gms

Other
1 stars 15 forks source link

make check for appearance of declared objects in modules ignore case #83

Closed fbenke-pik closed 9 months ago

fbenke-pik commented 9 months ago

When creating a matrix of appearance of declared variables in the various modules, this fix makes sure all appearances of the variable are included (also those with different capitalization).

codecov[bot] commented 9 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (9a604ba) 35.63% compared to head (161ec71) 35.63%.

Files Patch % Lines
R/checkAppearance.R 50.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #83 +/- ## ======================================= Coverage 35.63% 35.63% ======================================= Files 51 51 Lines 1664 1664 ======================================= Hits 593 593 Misses 1071 1071 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 9 months ago

Gives true positive on #82 :+1: Gives false positive on 'vm_demFeSector' is not addressed in all realizations of module 'industry'! :-1:

fbenke-pik commented 9 months ago

image

Why is this a false positive? Looks like in the fixed_shares module you only have vm_demFeSector_afterTax, but not vm_demFeSector.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 9 months ago

Well, my assessment was based on the assumption that the current version of the gms package (0.29.0 as of yesterday) would yield correct results. I assumed wrong.

Fixes false negative on 'vm_demFeSector' is not addressed in all realizations of module 'industry'! :+1:

Fixed.

fbenke-pik commented 9 months ago

Well, my assessment was based on the assumption that the current version of the gms package (0.29.0 as of yesterday) would yield correct results.

'vm_demFeSector' is not addressed in all realizations of module 'industry'! should be a second violation that was revealed as a side effect by the fix in this PR.

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 9 months ago

'vm_demFeSector' is not addressed in all realizations of module 'industry'! should be a second violation that was revealed as a side effect by the fix in this PR.

Yes.

But. Now codeCheck() gets its knickers in a twist:

2: Could not find any declaration for vm_demFEsector 
3: 'vm_demFEsector' is not addressed in all realizations of module 'regipol'! (none=0, regiCarbonPrice=1) (0 = missing, 1 = in code, 2 = in not_used.txt) 
4: 'vm_demFeSector' is not addressed in all realizations of module 'industry'! (fixed_shares=0, subsectors=1) (0 = missing, 1 = in code, 2 = in not_used.txt) 
5: 'vm_demFEsector' is not addressed in all realizations of module 'carbonprice'! (diffCurvPhaseIn2Lin=0, exogenous=0, expoLinear=0, exponential=0, linear=0, NDC=1, none=0, NPi=0, temperatureNotToExceed=0) (0 = missing, 1 = in code, 2 = in not_used.txt) 
6: 'vm_demFEsector' is not addressed in all realizations of module 'carbonpriceRegi'! (NDC=1, netZero=1, none=0) (0 = missing, 1 = in code, 2 = in not_used.txt) 
7: 'vm_demFEsector' is not addressed in all realizations of module 'tax'! (off=0, on=1) (0 = missing, 1 = in code, 2 = in not_used.txt) 
Execution halted

(This REMIND commit.)

So, not_used.txt should be checked case-insensitively, too.

fbenke-pik commented 9 months ago

Yes, not surprising to me as you added it with a different capitalization in not_used.txt as in declarations. Will cover that case if we decide to go down that route. Waiting for a decision on whether we want to universally enforce case-sensitive variable names .

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 9 months ago

Waiting for a decision on whether we want to optionally universally enforce case-sensitive variable names.

Fixed :p

fbenke-pik commented 9 months ago

Closed in favour of https://github.com/pik-piam/gms/pull/84

0UmfHxcvx5J7JoaOhFSs5mncnisTJJ6q commented 9 months ago

:facepalm: