Some comments inline!
On Thu, May 21, 2015 at 4:43 AM, Quim Gil <qgil(a)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(a)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-enfo…
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