codeCheck with less questions

[orichters]

• codeCheck is annoyingly asking too many questions, sometimes completely redundant ("are you sure").
• I put all "is not used at all, should I delete it from not_used.txt" into one single question
• For each module, I ask to add a variable/parameter to not_used.txt once, not once per realization.

In https://github.com/remindmodel/remind/blob/develop/modules/45_carbonprice/NPi/not_used.txt, replace vm_co2eq by pm_lab.

Run invisible(gms::codeCheck(strict = TRUE, interactive = TRUE). I omitted start and end, but paste just the selection process:

old:

Please choose choices:

1: yes
2: no
"pm_lab" appears in some not_used.txt files of module "carbonprice" but is not used in the code! Should it be removed?
Number or leave empty:
1
Selected: yes
"pm_lab" has been removed from not_used.txt files!



Please choose option:

1: yes
2: no
"vm_cesIO" is not addressed in realization "NPi_Baseline" of module "carbonprice"! Does that make sense?
Number or leave empty:
1
Selected: yes


Please choose option:

1: yes
2: no
"vm_cesIO" is a variable.",
                                            " Are you sure that it does not need to be treated in realization "NPi_Baseline" (e.g. fixed to a value)?
Number or leave empty:
1
Selected: yes


Please choose option:

1: yes
2: no
Should "vm_cesIO" be written to not_used.txt in realization "NPi_Baseline" of module "carbonprice"?
Number or leave empty:
1
Selected: yes
"vm_cesIO" has been written to not_used.txt!

new:

Please choose variables that should be deleted from 'not_used.txt', as they appear only there within the module (stated as 'group'):

              Group
1: pm_lab     carbonprice
2: Group: carbonprice

Numbers entered as 2,4:6,9 or leave empty:
1
Selected: pm_lab
'pm_lab' is omitted in all 16 not_used.txt files of module 'carbonprice'!

In module 'carbonprice', 'vm_cesIO' is not addressed in those 1 realizations: NPi_Baseline!
It is a variable and might need to be fixed to a value.
Does that make sense and it should be included in 'not_used.txt'? Y/n

[codecov]

Codecov Report

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

Comparison is base (23ada05) 34.88% compared to head (9722bd7) 35.93%.

Files	Patch %	Lines
R/codeCheck.R	4.76%	40 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
+ Coverage   34.88%   35.93%   +1.05%     
==========================================
  Files          51       51              
  Lines        1694     1650      -44     
==========================================
+ Hits          591      593       +2     
+ Misses       1103     1057      -46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tscheypidi]

I have to admit that I don't understand the new syntax. What would happen if I choose "2: group" in the first question? In is unclear what "group" means here.

In the second question I understand the question but I am not so sure if others not that familiar with it will understand it. In addition, I am skeptical that it is a good idea to have only a single question for all realization. I can imagine that in some cases the realizations are all basically the same in which case I understand why the aggregation to one question might be desirable, but in other cases the realizations will have very different implementations and the answer might be different on a per realization basis. Hence, I believe that merging the second case in one question is most likely not feasible / not a good idea.

[orichters]

• The group is the module, as stated above the list. We can format that differently, of course, suggestions welcome.
• If you choose 2 above, all parameters that are only found in not_used.txt files of the carbonprice module but never in the code, are removed. Here, it is only a single one, so this grouping seems rather useless. We can also omit the grouping and just print pm_lab in module carbonprice or so.
• regarding merging: I don't have a strong opinion on that.

[tscheypidi]

thanks for the explanations. Having thought about it a bit further I think we might even get rid of the first type of question completely: If an interface is removed and does not occur anymore anywhere in the module this is basically a clean up task which - to my understanding - comes with no risk. Hence, I think it would be fine if the function just removes these interfaces from not_used.

For the second case (not addressing an existing interface in some realizations) the situation is different and the questionaire is meant as a safety check that not addressing the interface is indeed the intended behavior.

[orichters]

• Yes, the first is just a clenaup. But I think also it is not bad to think again whether this change is correct and it can be quickly answered. I'm also fine with rearranging it into a pure yes/no question.
• Agree. When does this come up? I think there are two cases:
  • you added/updated a realization that now uses a new variable/parameter not yet used in this module. I think it is fine if you can answer whether this specific variable/parameter should be added to all the other not_used.txt in one go, not being asked 10 times, once for each old realization.
  • you added/updated a realization that does not use a variable that is already used in some other realization. That is one question anyway.

In these two cases, the structure of the question is fine, I think. What other application do you have in mind where you would like to be asked on a per-realization basis?

[LaviniaBaumstark]

• regardign the cleanup if an interface is deleted: I am in favour of deleting it automatically without any question. Developer will see those changes at least whern they look at the PR and can rethink it.
• regarding teh questions for a newly introduced interface: I see no additional cases. For me one quesiton would be fine

[orichters]

@tscheypidi: are you fine with just deleting variables from not_used.txt of all realizations without asking if the variable is not used (anymore) anywhere in this module?

[tscheypidi]

Yes

[orichters]

Ok, I implemented it such. I keep the info, but don't ask anymore.

Additionally, you are now asked why you want to omit it, and that is added into the not_used.txt. If you just press Enter as before, you get added by codeCheck.

Here is the result of replacing cm_co2eq by pm_lab in /p/tmp/oliverr/remind/modules/45_carbonprice/NDC/not_used.txt, then running devtools::load_all('/p/tmp/oliverr/gms'); invisible(gms::codeCheck(strict = TRUE, interactive = TRUE)) in /p/tmp/oliverr/remind.

 Running codeCheck...
 Finished data collection...            (time elapsed:   2.29)
 Naming conventions check done...       (time elapsed:   2.30)
  Running checkAppearance...
  Start variable matching...            (time elapsed:  0.063)
  Finished variable matching...         (time elapsed:   1.49)
 Investigated variable appearances...   (time elapsed:   3.85)
 Appearance and usage check done...     (time elapsed:   3.94)
 Switch Appearance check done...        (time elapsed:   4.31)
 Cleanup not_used.txt...                (time elapsed:   4.41)
'pm_lab' is omitted in all 15 not_used.txt files of module 'carbonprice'!

In module 'carbonprice', 'vm_cesIO' is not addressed in those 1 realizations: NDC!
It is a variable and might need to be fixed to a value.
To add this to 'not_used.txt', type the reason why this is correct, press Enter for a generic comment or type 'n' if that is wrong.
no impact of capital
 Interface collection and check done... (time elapsed:  58.92)
 Input folder check done...             (time elapsed:  58.92)
 Description check done...              (time elapsed:  58.93)
 All codeCheck tests passed!

not_used.txt now contains:

vm_cesIO,input,no impact of capital

[LaviniaBaumstark]

fine for me, if JPD agrees it can be merged

[orichters]

@tscheypidi, any comment / approval?

[tscheypidi]

I am not sure whether "omitted" is the right word in this case. I would think that something like "'pm_lab' has been removed as interface and has been deleted from all not_used.txt files of module 'carbonprice'!" might be easier to understand. Similarly, I could imagine that also the second formulation might lead to some confusions. To me something like the following feels more intuitive:

In module 'carbonprice', 'vm_cesIO' is not addressed in those 1 realizations: NDC!
It is an interface to other modules and might need to be taken care of in the realization (e.g. fixed to a value). If you are sure that it does not need to be addressed at all you can add this to 'not_used.txt'. Type the reason why it does not need to be addressed, or type 'n' if it actually needs to be addressed in the code.

Other than the wording the behavior looks good to me.

[orichters]

> devtools::load_all('/p/tmp/oliverr/gms'); invisible(gms::codeCheck(strict = TRUE, interactive = TRUE))
 Running codeCheck...
 Finished data collection...            (time elapsed:  53.45)
 Naming conventions check done...       (time elapsed:  53.45)
  Running checkAppearance...
  Start variable matching...            (time elapsed:  0.069)
  Finished variable matching...         (time elapsed:   1.63)
 Investigated variable appearances...   (time elapsed:  55.13)
 Appearance and usage check done...     (time elapsed:  55.23)
 Switch Appearance check done...        (time elapsed:  55.64)
 Cleanup not_used.txt...                (time elapsed:  55.73)
'pm_lab' has been removed as interface and has been deleted from all 15 not_used.txt files of module 'carbonprice'!

In module 'carbonprice', 'cm_expoLinear_yearStart' is not addressed in those 1 realizations: NDC.
If you are sure that it does not need to be addressed at all you can add this to 'not_used.txt'.
Type the reason why it does not need to be addressed, or type 'n' if it actually needs to be addressed in the code.
nobody needs you
 Interface collection and check done... (time elapsed: 140.38)
 Input folder check done...             (time elapsed: 140.38)
 Description check done...              (time elapsed: 140.39)
 All codeCheck tests passed!
> readLines("modules/45_carbonprice/NDC/not_used.txt")
 [1] "name,type,reason"
 [2] "cm_emiscen, switch, ???"
 [...]
[28] "cm_expoLinear_yearStart,input,nobody needs you"

[orichters]

Ok, I did as you suggested, @tscheypidi…

[tscheypidi]

thanks