Make Code-Review label values' meaning clear to non-maintainers
Currently the labels look like a direct suggestion to a maintainer dealing with the change, but even being interpreted this way, -1 label sounds strange as "didn't submit" doesn't match the fact nobody has submitted anything yet. What's worse, new contributors have every right to assume that the message is addressing them, and upon receiving an email saying "I would prefer you didn't submit this" they mistakenly think that the word "submit" refers to their action of posting for review, there's nothing to indicate this part is directed to the maintainer. Also, most people have never seen the "Submit" button, that's a process detail known to the maintainers but not the general public. This is confirmed by real-life observations of contributors to the OpenOCD project, I know at least three persons who got somewhat offended by receiving such a message from the review system. Change-Id: I035a615a6caf6e085edd5827f3bfe33342156324 Signed-off-by: Paul Fertser <fercerpav@gmail.com>
This commit is contained in:
parent
cec9b3b959
commit
2474e52beb
@ -19,7 +19,7 @@ correct'.
|
|||||||
|
|
||||||
The range of values is:
|
The range of values is:
|
||||||
|
|
||||||
* -2 Do not submit
|
* -2 This shall not be merged
|
||||||
+
|
+
|
||||||
The code is so horribly incorrect/buggy/broken that it must not be
|
The code is so horribly incorrect/buggy/broken that it must not be
|
||||||
submitted to this project, or to this branch. This value is valid
|
submitted to this project, or to this branch. This value is valid
|
||||||
@ -29,7 +29,7 @@ is submittable.
|
|||||||
+
|
+
|
||||||
*Any -2 blocks submit.*
|
*Any -2 blocks submit.*
|
||||||
|
|
||||||
* -1 I would prefer that you didn't submit this
|
* -1 I would prefer this is not merged as is
|
||||||
+
|
+
|
||||||
The code doesn't look right, or could be done differently, but
|
The code doesn't look right, or could be done differently, but
|
||||||
the reviewer is willing to live with it as-is if another reviewer
|
the reviewer is willing to live with it as-is if another reviewer
|
||||||
|
@ -518,8 +518,8 @@ describes the change.
|
|||||||
}
|
}
|
||||||
]
|
]
|
||||||
"values": {
|
"values": {
|
||||||
"-2": "Do not submit",
|
"-2": "This shall not be merged",
|
||||||
"-1": "I would prefer that you didn\u0027t submit this",
|
"-1": "I would prefer this is not merged as is",
|
||||||
" 0": "No score",
|
" 0": "No score",
|
||||||
"+1": "Looks good to me, but someone else must approve",
|
"+1": "Looks good to me, but someone else must approve",
|
||||||
"+2": "Looks good to me, approved"
|
"+2": "Looks good to me, approved"
|
||||||
@ -1444,8 +1444,8 @@ for the current patch set.
|
|||||||
}
|
}
|
||||||
]
|
]
|
||||||
"values": {
|
"values": {
|
||||||
"-2": "Do not submit",
|
"-2": "This shall not be merged",
|
||||||
"-1": "I would prefer that you didn\u0027t submit this",
|
"-1": "I would prefer this is not merged as is",
|
||||||
" 0": "No score",
|
" 0": "No score",
|
||||||
"+1": "Looks good to me, but someone else must approve",
|
"+1": "Looks good to me, but someone else must approve",
|
||||||
"+2": "Looks good to me, approved"
|
"+2": "Looks good to me, approved"
|
||||||
|
@ -213,8 +213,8 @@ public class AllProjectsCreator {
|
|||||||
new LabelValue((short) 2, "Looks good to me, approved"),
|
new LabelValue((short) 2, "Looks good to me, approved"),
|
||||||
new LabelValue((short) 1, "Looks good to me, but someone else must approve"),
|
new LabelValue((short) 1, "Looks good to me, but someone else must approve"),
|
||||||
new LabelValue((short) 0, "No score"),
|
new LabelValue((short) 0, "No score"),
|
||||||
new LabelValue((short) -1, "I would prefer that you didn't submit this"),
|
new LabelValue((short) -1, "I would prefer this is not merged as is"),
|
||||||
new LabelValue((short) -2, "Do not submit")));
|
new LabelValue((short) -2, "This shall not be merged")));
|
||||||
type.setAbbreviation("CR");
|
type.setAbbreviation("CR");
|
||||||
type.setCopyMinScore(true);
|
type.setCopyMinScore(true);
|
||||||
c.getLabelSections().put(type.getName(), type);
|
c.getLabelSections().put(type.getName(), type);
|
||||||
|
@ -70,8 +70,8 @@ public class Util {
|
|||||||
value(2, "Looks good to me, approved"),
|
value(2, "Looks good to me, approved"),
|
||||||
value(1, "Looks good to me, but someone else must approve"),
|
value(1, "Looks good to me, but someone else must approve"),
|
||||||
value(0, "No score"),
|
value(0, "No score"),
|
||||||
value(-1, "I would prefer that you didn't submit this"),
|
value(-1, "I would prefer this is not merged as is"),
|
||||||
value(-2, "Do not submit"));
|
value(-2, "This shall not be merged"));
|
||||||
|
|
||||||
public static LabelValue value(int value, String text) {
|
public static LabelValue value(int value, String text) {
|
||||||
return new LabelValue((short) value, text);
|
return new LabelValue((short) value, text);
|
||||||
|
Loading…
Reference in New Issue
Block a user