-
-
Notifications
You must be signed in to change notification settings - Fork 48
Allow requesting ANSI colors in stanc.js #1569
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
Conversation
src/stancjs/stancjs.ml
Outdated
| , (* NB: This should really be [Js.Unsafe.Inject (Js.array ...)] as above. | ||
| The fact that it is not is a historical mistake, and leads to the | ||
| resulting list in JS having a first entry of '0', the index. | ||
| Unfortunately, many tools have been built assuming the error message is | ||
| actually in the second entry, so we're stuck with this bad behavior. | ||
| *) | ||
| Js.Unsafe.inject (Array.map ~f:Js.string [| e |]) ) |
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.
So we have been using Unsafe.inject incorrectly? I think it would be better to write something like
Js.Unsafe.inject (Js.array (Array.map ~f:Js.string [| "0" ; e |]) )Or maybe just duplicate the error message?
let e = Js.string e in Js.Unsafe.inject (Js.array [| e ; e |])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.
Yes, more or less since the beginning. I only noticed this when building Stan Playground, and only spotted the fix while doing this change, but it's too late to really do much I think. Your suggestion of hard-coding the '0' seems reasonable to me given the choices.
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.
To elaborate a bit more, the 0 came from the data representation used in jsoo:
array block with tag 0 (e.g.
[|1;2|] => [0,1,2])
The missing Js.array call gets compiled down to caml_js_from_array, which is a 1-liner that returns .slice(1) of its argument
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.
How does Js.Unsafe.inject differ from Js.Unsafe.coerce? The latter seems tiny bit more more type-safe?
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.
They're exactly the same runtime code (both get compiled away to nothing), the difference seems to be that coerce requires the input be a Js.t already, which would have caught this.
I'll switch to using that, nice suggestion
Submission Checklist
Release notes
stanc.js can request ANSI escape code styling in errors and warnings by passing
color-outputas an argument. These are supported in most browser consoles.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)