Some comments inline!
On Thu, May 21, 2015 at 4:43 AM, Quim Gil qgil@wikimedia.org wrote:
Hi, thank you for this short and fresh review. Your help is welcome at https://phabricator.wikimedia.org/T597, where we are trying to identify blockers for using Arcanist, so we can discuss them and address them effectively.
Meanwhile, some comments here.
On Thu, May 21, 2015 at 9:01 AM, Ricordisamoa < ricordisamoa@openmailbox.org> wrote:
<review> <rant> Arcanist has to be manually cloned from Git and added to $PATH. Really?
Having seen how users struggle installing git-review and dependencies in their environments, I'm not sure this is a bad idea. Plus, I guess it makes updating to master pretty easy as well?
This isn't _that_ big a deal to me. git_review wasn't any easier to install. I'd prefer to have to install nothing but if I have to install something your description doesn't sound _that_ bad.
"Test Plan" is required.
Sounds like a good practice to me. Worst case scenario, type "I didn't test this patch at all."
We can turn this off, I think: http://stackoverflow.com/questions/20598026/how-do-i-disable-test-plan-enfor...
I suspect we _should_ turn it off too because we should be minimizing the number of changes we have to make when we switch tools. I'm not against requiring one for most commits at some point but that should be a separate thing. I should mention that I've never seen other open source projects require it, for what that is worth.
".arcconfig" should be automatically detected on git clone. I can't review my own revisions.
Neither should you, that is the point of code review. Then again, if there is no workaround for this, it might be a blocker for urgent Ops deployments (where we see many self-merged patches) and one-person projects. If this is the case, please create a blocker for T597 so we can discuss it in detail.
https://phabricator.wikimedia.org/T99905
I laid out my argument there but it goes the same as the test plan argument: we do it now and we shouldn't change to support the tool. We should change because we believe its a good idea.
Nik