Wpt: "Reviewable" is annoying and prevents seeing if something is successful or not

[domenic]

Although for some time we stopped using that old third-party code-review tool (Opera Critic I believe?), and had a brief period of bliss using just GitHub's interface, recently someone has reintroduced a new tool that edits our pull requests with a pointer out to some third-party service, "Reviewable".

This is not only intrusive and annoying, what with its giant purple badge taking up half of the OP of every pull request: it also hooks into the CI system, and prevents the CI from marking a pull request as "passing CI", because "1/2 checks have finished" (the second check, presumably, being whether someone has gone to that site and done whatever they want me to do). Indeed, if you look at the list of open PRs, there's not a green checkmark to be found anymore: https://github.com/w3c/web-platform-tests/pulls

I'd humbly request that whoever is so excited about using this tool please remove it from the test infrastructure and use it yourself in an opt-in manner, instead of foisting it upon all contributors.

I could be persuaded otherwise if we had evidence that some large percentage (e.g. 80%+) of pull requests received review through this tool. But my suspicion is that it's closer to 0%.

[jgraham]

Now that we have merged the infrastructure directly into the repository I want this for the more complex infra reviews. GitHub tools just aren't good enough yet.

[jgraham]

I've configured reviewable so that the review status is always shown as complete.

[annevk]

That's not a good solution. See e.g. how that makes it seem as if https://github.com/w3c/web-platform-tests/pull/5610 is okay to merge while it's clearly not.

[annevk]

Isn't there a way to make it only apply to infra patches or just add a comment "reviewable" to enable it for a certain PR? The majority of PRs shouldn't need this.

[domenic]

Yes, this doesn't address my point that if you really want this personally @jgraham, you can use it yourself, and not foist it upon all contributors. Please see my last paragraph especially.

[jgraham]

I think the argument that making this not required to pass status checks isn't a good solution because then the PR looks like it's OK to merge is a totally bogus argument; if we disabled it entirely then it would have exactly the same effect: the PR would be marked as OK to merge as soon as the travis checks passed.

I don't see what harm this is causing if it doesn't affect the status check. In particular the argument that it isn't always needed and therefore should be set up to be painful for the cases where it is needed vs being harmless in the case it isn't needed and easily available when it is required seems very weak.

[domenic]

This is causing harm in the same way advertising any other service that is irrelevant to contributors causes harm. How would you feel if we edited the infra to start advertising LastPass on every post, because I find it useful for managing my GitHub passwords? That's way more useful to me on any given PR (which I might be viewing from a computer without LastPass installed yet, and thus appreciate the helpful install link) than an advertisement for a tool I never use.

In the end we all use different tools for managing our GitHub experience: different email clients, review styles, notifications managers, issue trackers, Git GUI clients, ... Advertising your favorite one is not an appropriate use of web-platform-tests infra.

[jgraham]

I'm really trying to make things better here but I'm not motivated to spend time on it when you make such disingenuous comparisons.

[domenic]

Yes, and I'm really trying to contribute to web-platform-tests, but I'm not motivated to spend time on it when any PR I make becomes a nascar for your favorite web services.

[jgraham]

The idea that reviewable is my favourite web service is hilarious. But unlike GitHub it provides a review UI that's appropriate for larger changes and as such is preferred for many infrastructure changes. I would love to replace the badge with a simple link or for GitHub to have some explicit UI for this kind of external link (or for GitHub Review UI to have the same level of functionality). However I continue to believe that the harm from having this icon in the review is minimal (so far you are the only person who complained about it across multiple projects including others where it it is not used for trivial reviews), but for cases where it's an appropriate tool it's very useful to have a direct link to the review.

Is there any solution that would work for you other than disabling this entirely (which is clearly not acceptable to me)?

[domenic]

Any way of making it opt-in, so that you can use it on PRs you want to use it on, but it never appears for the rest of us, would be acceptable.

[tobie]

Yeah, pushing such tools onto new contributors (that aren't aware they can ignore them) steepens the learning curve of the project. It also makes it unclear for everyone where reviews are happening. We've had this discussion many times in the past already. I'm glad to see others voicing their concerns here.

[tobie]

Related issue: https://github.com/Reviewable/Reviewable/issues/471

[tobie]

Alright, so @wpt-pr-bot now removes the reviewable badge in all PRs but those that touch the tools and resources directories.

The cleanup gets triggered by new comments or edits to the OP (normally), so we should see a gradual cleanup over the upcoming days.

[domenic]

That's amazing!! What a great solution.

[bobholt]

Based on @domenic's comment above, I'm going to assume this was an acceptable solution to the issue and close. Feel free to reopen if I've misread.