Make DEP a yellow waiting dot instead of a red cross.

[iphydf]

The red cross tells people something failed, but actually it's just waiting. Make this more like WIP, which is a yellow waiting dot for WIP PRs. The effect is the same: can't be submitted, but the message is better.

[z0al]

Hi, @iphydf thanks for opening this!

DEP follows the way GitHub status API works. The app has to report one of 4 states: error, failure, pending, or success per check.

• Failed state means the assertion that made by that app wasn't true, for example, Travis reports "fail" we the script (mostly tests) exists with a non-zero code.

• Pending state basically means "I'm working on it". It only meant to tell the developer that the app received the event but would take some time to report the final result.

• Error state means the "check" couldn't run because of e.g. misconfigured params.

• Success state means the "checks" passed.

The red cross tells people something failed

I understand that might be confusing but GitHub shows the "red cross" sign for both "failed" and "error" states. This can't be controlled by the app (DEP in this case).

However, currently, DEP doesn't report error state, so the "red cross" always means some dependencies are still unresolved.

That being said, I think reporting pending instead of failure would be more confusing.

Sorry, but can't help much on this. The app should follow the way GitHub integrations work.

[iphydf]

Ok, I'll keep using Reviewable, WIP, and other "broken" apps then, and can't use this correctly implemented DEP app. I understand that the states are described in that way, and I agree with your interpretation, but it doesn't match the reality (such is the life on Github, I am very aware, given that I create my own integrations as well).

[z0al]

Ok, I'll keep using Reviewable, WIP, and other "broken" apps then

I wasn't aware of those apps. And I didn't mean those apps are "broken". I'm just trying to make the app works the way GitHub users expect - I assume.

[ryanhiebert]

I'm curious, why is it not acceptable for it to be a red X in your scenario? I know that sometimes I haven't liked to do "changes requested" reviews, because they use the same red X, and I'm worried that it will look scary to the requester.

But it doesn't mean that, and with the words we use we can be clear that we're happy for their contribution, and they're well on the way, just something needs to happen first. That's the same here. It's not that it cannot be accepted, it's that something needs to happen first. That red x is more of a sign to tell the maintainers that "this isn't ready yet, do not merge it" than "this is a failure of a pull request and should never be considered again". That would be a closed PR.

I think we do ourselves and contributors a disservice by being scared of the red X. That's my general philosophy on it, and you may have further constraints that force you to avoid it anyway, but that's my thinking on why that Red X, even though it can look a bit scary at times, is the best thing to use here, and other similar places. We shouldn't be scared of using it.

[iphydf]

The red X means one of the checks failed. In our workflows, this almost always means something actionable for the contributor. The maintainers look at PRs with a yellow dot, because those have probably passed checks. On rare occasions, a yellow PR is in fact not ready for review, and then it's either explicitly marked as WIP, or we find that travis is running (but that's done after 3-4 minutes, so it should still be ready for review).

This is not about scaring users, it's to make life easier for maintainers. Looking at https://github.com/TokTok/c-toxcore/pulls (or the soon-to-be-fixed global pull request dashboard), I and other reviewers can tell at a glance which PRs might need review (and personally I mostly ignore the colourful tags that some maintainers put on there). All PRs that currenty have the red X on them, except the "Make conferences persist ..." one are in the state "action required from submitter". The "Make conf.." PR is dependent on another PR, but ready for review, no action required from submitter, it's just waiting.

Maybe this is misinterpreting Github's intentions for the statuses, but we make the tools we use work the way we can.

[ryanhiebert]

I decided to see if I could look up the semantics that GitHub defines, which of course is what we'd want to follow. I'm afraid that I haven't been able to find any clearly defined semantics in the commit statuses documentation. It just says the 4 types of statuses that may be set, it doesn't give any guidelines, that I've found, on when to use one or the other.

My intuition has been the same as the maintainer of this project. But I also see where you're coming from @iphydf , and I don't know that your view of it is strictly wrong either. With some guidance from GitHub, this might be an clearer choice to make.

Clearly GitHub isn't all that worried about stuff being "pending" for a long time. Reviews can potentially be pending for quite a while.

I typically see "pending" statuses as meaning (with my maintainer hat on) "don't look at this just yet, some of the automated checks are still running". I think this is likely consistent with what GitHub sees as the correct interpretation, and to demonstrate that I offer as evidence that the "pending" state (or approved or changes required, even) do not show up as the icon in the list of pull requests. Since any pending state will mark it as pending, unless there's an error, they seem to think that it's ok to use a checkmark to indicate that things are ready to dig into from the list, and I'd be inclined to agree with that assessment as a general rule.

It's also definitely true that other review 3rd party apps have used the pending state, which doesn't get the same special treatment as GitHub's reviews, as meaning "a review is needed". So I'm not sure there's a clear answer, unfortunately.

---

A less than ideal possibility might be to allow the status put on the commit to be configured by the repository. Probably the easiest way to do that would be to have it in a configuration file like we do for .travis.yml. I'm not keen on the idea of adding configuration, but if we do, that's probably the most "Right" way to do it.

[iphydf]

Interesting, so what you're saying is that "pending" overrides "failed"? I thought any "failed" check will set the red cross, regardless of whether other things are pending or not.

[ryanhiebert]

No, it won't override, I just wrote imprecisely when I said "error". I meant "error or failure", sorry.

[z0al]

A less than ideal possibility might be to allow the status put on the commit to be configured by the repository. Probably the easiest way to do that would be to have it in a configuration file like we do for .travis.yml. I'm not keen on the idea of adding configuration, but if we do, that's probably the most "Right" way to do it.

I took this idea but from a different perspective and built an app that lets you choose when a pull request is ready for review based on its statuses.

Sample config

# File: .github/review-me.yml
when:
  continuous-integration/travis-ci/pr: success
  wip: pending
  dep: failure

# Override default label (optional)
label: 'Review Me'

It simply adds the review label if all when conditions matched the pull request's head statuses. Otherwise, it removes the label (if any).

Hope it helps regarding this issue.

Full source/docs: https://github.com/ahmed-taj/probot-review-me

[iphydf]

Hey nice :) that'll be useful, thanks!

[ryanhiebert]

I like it. Good work @ahmed-taj !