Skip to content
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

Exit process with 1 when Flow check fails? #147

Open
migueloller opened this issue May 1, 2018 · 5 comments
Open

Exit process with 1 when Flow check fails? #147

migueloller opened this issue May 1, 2018 · 5 comments

Comments

@migueloller
Copy link

Currently it's possible to set a coverage threshold. It would be nice if there was another option that made it required for the Flow check to pass.

@iduuck
Copy link

iduuck commented Jun 17, 2018

Any update here, @rpl?

@iduuck
Copy link

iduuck commented Jun 18, 2018

@migueloller Since @rpl doesn't seem to have a look at the issues, I have forked the project and implemented the exit code for flow: https://github.com/fintory/flow-coverage-report

@migueloller
Copy link
Author

migueloller commented Jun 18, 2018

@iduuck, that looks great! Thanks for taking the initiative 😄

I'm not sure how responsive @rpl is to PRs but I'm sure it would be helpful if you submit one with the changes done in fintory@9c4a38c. You can likely just setup a branch, cherry-pick that commit and submit the PR. It'll get my +1 for sure!

@rpl
Copy link
Owner

rpl commented Jun 19, 2018

@migueloller @iduuck I personally prefer to have a flow-check npm script that fails when there are flowtype errors and leave the report generator to only fail because the coverage is lower than the desired threshold.

But I'm not against supporting the behavior described in #147 (comment) and I would be more than happy on integrating it as an additional configurable behavior (enabled by an additional cli/config option, instead of being the default behavior, and possibly guarded by a new test case to prevent the new option and related behavior from regressing in the future, the modules around the cli argument parsing based on yargs are unfortunately the parts on which the flow coverage is lower and so, a bit ironically :-P, flow doesn't help a lot there).

@iduuck
Copy link

iduuck commented Jun 20, 2018

Ok, I did a PR (#158) for the default behaviour, but will add a config/cli option when I have some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants