Fix missing include_paths when using mod$check_syntax()

[mike-lawrence]

Submission Checklist

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

Summary

I was looking at the code for mod$check_syntax() while tracking down a different issue, and noticed that while it has an include_paths argument that is used in building stancflags_val, this latter variable isn't actually used anywhere, yielding errors when the model indeed has includes.

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):
Mike Lawrence

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)

[jgabry]

Thanks Mike! Let us know if you need some help figuring out the test failures. Does this work interactively for you now and it's just the unit tests failing?

[mike-lawrence]

Ah, odd, passed all tests locally; I'll dig in...

[jgabry]

I haven't gone through the results but it's also possible something is messed up with github actions. That happens sometimes, so if it seems like everything is correct then we could just need to restart the tests or wait for whatever the issue is to get resolved.

…

On Mon, Nov 2, 2020 at 12:53 PM Mike Lawrence ***@***.***> wrote: Ah, odd, passed all tests locally; I'll dig in... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#335 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3PQQ3BSMDSQIAFMSGUJDDSN4E4FANCNFSM4THYNSUA> .

[mike-lawrence]

Oh, I lie, there's a couple I missed (partly due to not realizing that one needs to re-install before running the tests).

[mike-lawrence]

Ok, I'm going a bit crazy here. check_syntax() now works for models with includes, but not for any other model, yet when I manually run the same stanc call (now printed via echo_cmd) in a terminal, it runs fine. I'm actually getting similarly odd behaviour with my other PR, so I wonder if something is wonky on my local system (Ubuntu 20.04; cmdstan 2.25.0).

[rok-cesnovar]

The problem is that stancflags_val is "" when includes are not used so it tries to add "" as a argument to stanc3 which I guess is what is causing problems here. That should be NULL.

Once that is fix this is good to go.

[rok-cesnovar]

Suggested change

	stancflags_val <- trimws(paste0(stancflags_val, include_paths_flag, include_paths, " "))
	stancflags_val <- trimws(paste0(include_paths_flag, include_paths, " "))

[rok-cesnovar]

stancflags_val is always going to be "" so it can be removed inside paste0. Also change stancflags_val <- "" to stancflags_val <- NULL. That makes c(self$stan_file(), stanc_built_options, stancflags_val) the same as c(self$stan_file(), stanc_built_options) if no include flags are used.

[mike-lawrence]

There must be something odd with how processx::run() parses the value supplied to its args argument, as @rok-cesnovar's code didn't resolve the issue and I ended up having to put a conditional on the creation of the value supplied to that argument. Passes all tests in the model-compile group now, just running the full suite of tests to make sure it's good to go.

[mike-lawrence]

Ok, it's passing all tests locally except for some tests in the install group, which look to be unrelated to the PR entirely.

[codecov-io]

Codecov Report

Merging #335 into master will decrease coverage by 0.63%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
- Coverage   90.15%   89.52%   -0.64%     
==========================================
  Files          12       12              
  Lines        2427     2586     +159     
==========================================
+ Hits         2188     2315     +127     
- Misses        239      271      +32     

Impacted Files	Coverage Δ
R/model.R	91.92% <100.00%> (+3.70%)	⬆️
R/install.R	54.72% <0.00%> (-0.67%)	⬇️
R/args.R	99.54% <0.00%> (-0.23%)	⬇️
R/data.R	100.00% <0.00%> (ø)
R/read_csv.R	96.31% <0.00%> (+0.01%)	⬆️
R/utils.R	72.06% <0.00%> (+1.28%)	⬆️
R/run.R	98.84% <0.00%> (+1.71%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbebd3e...5fe1fa9. Read the comment docs.

[rok-cesnovar]

This looks good to me. Thanks for doing this.

@mike-lawrence if you don't mind I am going to add you to the list of contributors. Will push that to this branch along with some stylistic changes if you agree. Thanks again!

[mike-lawrence]

Works for me!

[mike-lawrence]

Actually, I think I just put the finishing touches over on the only-recompile-on-functional-code-change PR, which also solves this. Unfortunately, working on the two PRs in parallel and being a git beginner, I wasn't able to keep things aligned, and I'm not sure if merging this now will make for a headache in merging the other PR later. The other PR is currently stalled thanks to a failing test that I'm pretty sure is attributable to a bug in stanc. Anyway, thought I'd post a heads up here in case you wanted to just wait for the other PR to resolve.

[rok-cesnovar]

Sorry for the delay, will merge this as soon as tests pass.

[rok-cesnovar]

Congrats @mike-lawrence on your first merged pull request! 🎉