Page MenuHomePhabricator

Fix/revert #Patch-needs-improvement Phabricator project tag
Closed, ResolvedPublic


Patch-Needs-Improvement was created without prior discussion (or I could not find it) and without logging in T103700, which is both required by the guidelines.
The tag has no description at all, not explaining how to use it in a consistent way, together with or in contrast to Patch-For-Review.
It seems to be set entirely manual, potentially leading to more noise if not manually removed again once someone recognizes that the patch in Gerrit was updated.

There are also requests to remove Patch-For-Review completely in T104413 hence Patch-Needs-Improvement might just add more controversy.

Please fix.

Event Timeline

Aklapper created this task.Mar 8 2016, 2:21 PM
Luke081515 triaged this task as High priority.Mar 8 2016, 4:26 PM
Luke081515 moved this task from Incoming to Administration on the Project-Admins board.
Luke081515 added a subscriber: Luke081515.

@Jdlrobson: Can you please reply here? Otherwise I'd likely temporarily disable this project. Thank you!

Gerrit is not a useful tool for code review. We want to ensure we review people's patches promptly.

In an attempt to improve code review we have created a dashboard to track patches that have been submitted that need to take care of

It has been effective so far - only 7 patches are open across reading web's 18 extensions:

We tried using "stalled" as a way to signal that something needed input before being put back in the review queue. This didn't work so well, so we are now experimenting with a tag which seems clearer. It doesn't seem to have caused any problems from a logistical point of view yet.

Projects are cheap and I didn't think project creation rules applied to tags. Most systems allow you to input any arbitrary tags you want e.g. Flickr, Twitter - this is why tags are useful.

@Jdlrobson: Could you please add a tag description, not explaining how to use it in a consistent way, together with or in contrast to Patch-For-Review? Thanks.

I already did but I don't understand why given has no description and tags don't usually have descriptions in all other websites. The whole purpose of a tag is to convey information in the tag itself so that it doesn't need a description.

For this to work fully I think we need gerritbot add the tag when the patch is CR-1/-2 or V-1, and have the same bot to remove it when the patch is ok or is merged. Otherwise, having the bot saying "Code-Review-1/-2//Verified-1 by <username> on submitted patch" might be better.

tags don't usually have descriptions in all other websites

[citation needed]

Aklapper closed this task as Resolved.Apr 3 2016, 11:04 AM

Thanks! I think we're fine here now that the tag has a description. Let's see how this tag will be used in the next weeks and months.
Closing as resolved.