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
Fix missing include_paths when using mod$check_syntax() #335
Fix missing include_paths when using mod$check_syntax() #335
Conversation
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? |
Ah, odd, passed all tests locally; I'll dig in... |
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>
.
|
Oh, I lie, there's a couple I missed (partly due to not realizing that one needs to re-install before running the tests). |
Ok, I'm going a bit crazy here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
R/model.R
Outdated
@@ -578,7 +578,7 @@ check_syntax_method <- function(quiet = FALSE, | |||
} else { | |||
include_paths_flag <- " --include_paths=" | |||
} | |||
stancflags_val <- paste0(stancflags_val, include_paths_flag, include_paths, " ") | |||
stancflags_val <- trimws(paste0(stancflags_val, include_paths_flag, include_paths, " ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stancflags_val <- trimws(paste0(stancflags_val, include_paths_flag, include_paths, " ")) | |
stancflags_val <- trimws(paste0(include_paths_flag, include_paths, " ")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There must be something odd with how |
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 Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Works for me! |
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. |
add Mike as contributor add to news.md # Conflicts: # NEWS.md
Sorry for the delay, will merge this as soon as tests pass. |
Congrats @mike-lawrence on your first merged pull request! 🎉 |
Submission Checklist
Summary
I was looking at the code for
mod$check_syntax()
while tracking down a different issue, and noticed that while it has aninclude_paths
argument that is used in buildingstancflags_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: