Do not print stdout and stderr in stanc.js - #762
Conversation
|
|
||
| let stan2cpp_wrapped name code (flags : Js.string_array Js.t Js.opt) = | ||
| stanc_stdout := "" ; | ||
| stanc_stderr := "" ; |
There was a problem hiding this comment.
We have to reset every time because these are global variables. If we don't, we just accumulate warnings from all previous compilations.
There was a problem hiding this comment.
A better pattern would be to have the components return warnings and errors rather than printing them. Could we do that instead or would that mess up the timing for a release or something?
There was a problem hiding this comment.
That was my first plan, but I quickly realized that is a pretty big refactor. Particulary not sure on how to return from the parser for example.
| cpp ) | ||
|
|
||
| let stanc_stdout = ref "" | ||
| let stanc_stderr = ref "" |
There was a problem hiding this comment.
Would it be better to use a list or something?
There was a problem hiding this comment.
yeah, a list would be constant time to append to instead of O(N)
|
@seantalts sorry to keep tagging you with these stanc.js PRs, not sure who else knows enough about this. If anyone else feels they can review this, feel free. |
|
Let's hope this is the last wall for rstan to cross over... this is getting heroic to make it work. |
|
The critical stuff should hopefully now be squashed. We need to find a better way to support includes but that can be workaround for now. |
|
We should make bets on a 2020 or a 2021 rstan 2.25... |
|
Bump. Anyone up for reviewing this? |
|
Taking a look! |
| cpp ) | ||
|
|
||
| let stanc_stdout = ref "" | ||
| let stanc_stderr = ref "" |
There was a problem hiding this comment.
yeah, a list would be constant time to append to instead of O(N)
|
|
||
| let stan2cpp_wrapped name code (flags : Js.string_array Js.t Js.opt) = | ||
| stanc_stdout := "" ; | ||
| stanc_stderr := "" ; |
There was a problem hiding this comment.
A better pattern would be to have the components return warnings and errors rather than printing them. Could we do that instead or would that mess up the timing for a release or something?
| "Semantic check failed but reported no errors. This should never \ | ||
| happen." | ||
| in | ||
| if is_flag_set "version" then Result.Ok (Fmt.strf "%s" version, []) |
There was a problem hiding this comment.
I think we want to do something to keep the ability for phases of the compiler to return warnings rather than delete these for-now empty lists.
|
Closing in favor of #777 |
3 participants
Fixes #741
Stdout and stderr in stanc.js are rerouted to a ref (is there a more Ocamlish solution?) and the contents of stderr are returned as the warnings element of the return object. Stdout is ignored.
I think this is the more Javascript way of doing it and for use in Rstan we have to avoid printing anyways (see issue).
Currently, in order to print pedantic warnings in a browser, I had to reroute windows.console.error which is not how we want our API to work.
Release notes
Removed stdout and stderr in stanc.js.
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)