Conversation
|
/сс @mroch |
mroch
left a comment
mroch
left a comment
There was a problem hiding this comment.
cool!
how noisy will this be? we need some settings (parser options, flowconfig options, etc) and this could be one of them if we need to toggle it off.
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 |
3 participants
Fixes #2854