Alt: #include for stancjs, simpler implementation#1433
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1433 +/- ##
==========================================
+ Coverage 89.83% 89.88% +0.05%
==========================================
Files 63 63
Lines 10484 10497 +13
==========================================
+ Hits 9418 9435 +17
+ Misses 1066 1062 -4
|
test/stancjs/stancjs.expected
Outdated
| Syntax error in 'include/a.stan', line 1, column 0, included from | ||
| 'include/b.stan', line 1, column 0, included from | ||
| 'include/a.stan', line 1, column 0, included from | ||
| 'string', line 1, column 0, include error: | ||
| ------------------------------------------------- | ||
| 1: #include <include/a.stan> | ||
| ^ | ||
| ------------------------------------------------- | ||
|
|
||
| File include/b.stan recursively included itself. |
There was a problem hiding this comment.
Even though it says that the error occurred in file include/a.stan the snipped is from include/b.stan (or rather string which happens to be identical to include/b.stan; Errors.pp does not have access to the includes map.)
There was a problem hiding this comment.
Errors.pp doesn't need it, this is because of the lack of the filename-in-msg argument. I have thought a couple times about making that argument essentially required in stancjs, because it's almost always wrong for a caller to not specify it. I've updated the test to read better.
There was a problem hiding this comment.
You misunderstood the issue. (This recursive include was a poor example of the general pattern.) Change the includes in the test to
var recursive_a = `// comment here\n#include <include/b.stan>`
var recursive_b = `#include <include/a.stan>\n // comment here`and you'll see the caret points to nonsense.
Errors.pp calls Errors.pp_context_with_message which reads from the filesystem. It would be possible for whoever calls Errors.pp to check the filename and supply the correct file, but that's not what currently happens, and in my opinion it we should keep the file-finding logic inside Errors.pp_context_with_message.
There was a problem hiding this comment.
Ah, thanks for that example.
Let me know what you think of my latest commit. To break a circular dependency I needed to move the ref cell out of Preprocessor, but I think the result is decent.
Node tests don't provide coverage information
nhuurre
left a comment
nhuurre
left a comment
There was a problem hiding this comment.
Looks good, just a couple of nitpicks.
2 participants
Alternative to #1432, also closes #1430
Rather than using functors, this just makes it so the
include_pathsvariable in the Preprocessor now looks likeAnd this is used in the preprocessor to switch between the two styles.
Submission Checklist
Release notes
stanc.jscan now accept models which contain#includestatements. A fourth argument is available on the javascript stanc function which must be eitherundefinedor a object mapping included file names to Stan source code as strings.Copyright and Licensing
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)