Skip to content

Don't overwrite the codecov/project status to green#7712

Closed
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:codecov-patch-enforce
Closed

Don't overwrite the codecov/project status to green#7712
wilzbach wants to merge 1 commit intodlang:masterfrom
wilzbach:codecov-patch-enforce

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jan 15, 2018

Now that we CodeCov properly find the base commit, it will only error if the patch reduces the coverage of a file, which shouldn't happen too often.

Follow-up to #7711

Now that we CodeCov properly find the base commit, it will only error if the patch reduces the coverage of a file, which shouldn't happen too often.
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

@wilzbach
Copy link
Contributor Author

So I went through page 1 of the open PRs and those two PRs would have been flagged:

So it doesn't work well for refactorings or work with CTFE, so maybe sticking with informational: true and just being a bit more careful when looking at the diff is the best?

@JinShil
Copy link
Contributor

JinShil commented Jan 15, 2018

I'm torn on this. I want it to be a requirement, but that red X is going to be just an annoyance if I have to visit each PR to see if it is only the codecov that's failing. I have to make a binary choice, so I vote "No".

@WalterBright
Copy link
Member

As noted, it is impractical to require this. There's no substitute for the reviewer checking the uncovered new lines, and requiring that the ones that could be covered be covered, but these are a judgement call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants