| eadler | |
| Jan 27 2016, 10:45 PM |
| F1079865: happycat.jpg | |
| Jan 29 2016, 12:59 AM |
Steps to reproduce:
What you see:
Revision 'D42: Remove obsolete best practices for code review.' has not
been accepted. Continue anyway? [y/N] n
I'd expect this to either go through, or for the error message to be more explicit.
Would this phrasing be more clear?
The state of Revision X is not "Accepted". Continue anyway? [y/N]
Yes it would. I might be more explicit and say which state it actually is in.
FWIW this is a real confusion that one of the engineers currently working on deploying phabricator recently encountered.
That would be more clear. Even better would probably be to say what is is in fact *in*. Such as:
The state of Revision X is not "Accepted" (it is "Changes Planned"). Continue anyway? [y/N]
If you don't have the model internalized in your head, it may not be immediately obvious that being marked as changes pending is relevant in this - though the new phrasing you proposed makes it a lot more likely that one would realize I think, than the current phrasing.
I've have several thousand or so diffs in Phabricator throughout the years, can't say I've ever mistaken something I planned changes to as landable.
This is almost certainly something that is only confusing to entirely new users who aren't yet familiar. I'm not surprised you're not confused if you've used phabricator for a non-trivial amount of time.
In PHI231, a more experienced user went through this workflow:
This isn't a bug (revisions intentionally stay in "Plan Changes" once put there until authors kick them back out) but the confusion is reasonable.
We can likely improve both issues by special casing this prompt into something like this:
This revision is currently in the state "Changes Planned", indicating that you expect to revise it before landing it. Normally, you should either update it with revised changes or "Request Review" to submit it for review as-is, then wait until it is "Accepted" by reviewers before you land it.
Land revision in state "Changes Planned" anyway? [y/N]
A related improvement is mirroring the existing "This revision is now accepted and ready to land." transaction when revisions land from a state other than "Accepted", e.g.:
/!\ This revision was not fully reviewed, and landed while in state "Changes Planned".
The wordsmithing could maybe be a little more clear there and it would be nice to explicitly say "we expected 'Accepted'" while still sounding human, but you currently have to look for the absence of a "This revision is now accepted..." transaction which no one can reasonably know.
