-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Enforce 100% coverage #4120
Comments
I agree, this is easy to mess up; Flow tends to silently ignore types it can't figure out and it can be tough to tell what has been missed. It helps very much to use an editor plugin that shows coverage, but it should be possible to do this well via the CLI or configuration as well. |
All we need right now is flow strict mode directive because it's easy to break something without warning. |
100% is sometimes hard to reach, however coverage regressions would be nice indeed. I think this can be implemented as a third-party solution. A pre-commit script could run coverage on all |
@steida, there are still things that you cannot annotate: try {
} catch (e) {} //you cannot annotate e here ever
export default {
myMethod () {
this.body = 3 //you cannot annotate this
}
} I use second structure with |
@nickmessing That's fine. The purpose is different. Sometimes we have 100% coverage only because of type inference. When something is broken, coverage goes down without error. cc @calebmer |
I’m glad to hear that 100% coverage is desirable 😊 We encourage you to get coverage information from Flow and add your own rules to stop the build if coverage isn’t satisfactory. To get coverage information run:
You can see Nuclide use it here: https://github.com/facebook/nuclide/blob/9a380e8c605dd68a8b70ed75cf655629c185166b/pkg/nuclide-flow-rpc/lib/FlowSingleProjectLanguageService.js#L424 |
Simple stupid |
I agree with @steida 100% flow coverage is desirable. I, however, cannot enforce 100% flow coverage completely because of two reasons currently
A current workaround could be writing a custom script that checks all changed files and looks in a JSON file which maps thresholds for specific files. But that can easily be abused. |
@k @nickmessing right now we have an export function coerceThrowToReturn<T> (f: () => T): T | Error {
try {
return f()
} catch (untypedError) {
if (untypedError instanceof Error) {
return untypedError
} else {
return new Error(untypedError)
}
}
} Then we have to do a So far, that's literally all we've needed to achieve 100% type coverage, and we enforce that with |
@k I guess |
@svsool I tried importing both ways and have the same problem. The the internals of the compose are typed but flow-coverage thinks the compose function itself is untyped @LoganBarnett good idea! I will use that trick once we figure out a solution to the untyped compose function. |
@k https://github.com/LoganBarnett/error-as-either also includes the types in a place Flow can find. It's basically a packaged version of the function in my earlier comment. Then you don't need to make exceptions in your coverage report. |
Something like jest |
I guess addressing #2981 would cover this issue |
Until we get natively support we can use eslint-plugin-flowtype-errors/enforce-min-coverage rule. |
// @flow-strict
Why? Because there are many cases where 100% type coverage needs a lot of type boilerplate, but without it, Flow somehow still detects wrong types, but only coverage is smaller. Enforcing 100% coverage would help I suppose.
The text was updated successfully, but these errors were encountered: