Add coverage to Try Flow #7871
Add coverage to Try Flow #7871
Conversation
|
/сс @mroch |
src/flow_dot_js.ml
Outdated
| let root = Path.dummy_path in | ||
| let content = Js.to_string js_content in | ||
| match parse_content filename content with | ||
| | Error _ -> failwith "parse error" |
There was a problem hiding this comment.
this seems too easy to hit. I'd say make it return a JSON error instead
There was a problem hiding this comment.
maybe raise_js_error?
| const flow = Promise.resolve(editor.getOption('flow')); | ||
| const sources = Promise.all([ | ||
| flow | ||
| .then(flowProxy => flowProxy.coverage('-', text)) |
There was a problem hiding this comment.
this code has to run on older versions of flow. I guess the catch will handle that, but swallowing errors always comes back to bite me. if it returns a JSON error for parse errors instead of throwing, then you could return a similar error if flow.coverage == null.
There was a problem hiding this comment.
How is this different from catch?
There was a problem hiding this comment.
not catching inside Promise.all, would kill whole sequence, checkContent wouldn't be called then
even if it is user-defined error
There was a problem hiding this comment.
swallowing errors always comes back to bite me.
type-at-pos bites back, if you add console.error to type-at-pos catch it would complain about re_string_match not implemented 😬
goodmind
force-pushed
the
try-flow-coverage
branch
from
84fb954 to
e90d123
Compare
|
@mroch I added coverage checkbox |
Fixes #2854