Improve handling of user header

[martinmodrak]

Submission Checklist

• Run unit tests
• Declare copyright holder and agree to license (see below)

Summary

Resolves #812 and #813 (as the fix to #813 requires changes needed to fix #812, I've made a single PR for both)

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Institute of Microbiology of the Czech Academy of Sciences

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

• Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[codecov-commenter]

Codecov Report

Merging #818 (e7a935f) into master (e4ff5d4) will increase coverage by 0.14%.
Report is 12 commits behind head on master.
The diff coverage is 92.85%.

❗ Current head e7a935f differs from pull request most recent head f4a7fb2. Consider uploading reports for the commit f4a7fb2 to get more accurate results

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
+ Coverage   88.26%   88.40%   +0.14%     
==========================================
  Files          12       12              
  Lines        4166     4201      +35     
==========================================
+ Hits         3677     3714      +37     
+ Misses        489      487       -2     

Files Changed	Coverage Δ
R/model.R	91.12% <91.17%> (+0.50%)	⬆️
R/utils.R	74.50% <95.45%> (+0.63%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[martinmodrak]

@rok-cesnovar @jgabry So this appears ready to merge. Beyond the changes to header handling I made some tweaks to tests. In particular, there are now expect_compile and expect_no_recompilation helpers that standardize the behaviour to have file existence and modifications time checked at all places and not rely only on messages (which I am not sure are actually checked when testing within the CI environment as they are bound to interactive mode). I've also added a dry_run argument to $compile that let's us test argument handling and messages without actually trigerring the C++ compilation. Both could probably be used more extensively, but I didn't want to pollute the PR too much. If you like the concept, I will open a separate issue + PR on applying this more broadly.

[jgabry]

Thanks @martinmodrak! I made a few suggestions but I'm just being nitpicky to match style, no substantive changes. @rok-cesnovar has written much more of the compile method code than I have so let's see if he has any other feedback, but this looks good to me.

I've also added a dry_run argument to $compile that let's us test argument handling and messages without actually trigerring the C++ compilation.

Good idea.

Both could probably be used more extensively, but I didn't want to pollute the PR too much. If you like the concept, I will open a separate issue + PR on applying this more broadly.

Great, yeah I like the concept.

[rok-cesnovar]

Thanks!

[martinmodrak]

Hi, just merged master back in. Could somebody merge this before it gets out of sync 🙏 ? Or is there something else to be done/approved?

[rok-cesnovar]

I just assume the author can merge once its approved :) Thanks Martin!