<review> <rant> Arcanist has to be manually cloned from Git and added to $PATH. Really? "Test Plan" is required. ".arcconfig" should be automatically detected on git clone. I can't review my own revisions. "Lint" and "Unit" are shown as completely different processes. Diffs all over the page clutter the UI. No powerful plain-text Gerrit-like queries. I have to click "Edit Revision" to add reviewers. No -2/-1/+1/+2. WTF? </rant> <yay> Tokens! Comment preview! Can paste raw diffs! </yay> <summary> Some nice features aren't worth a change of workflow. </summary> </review>
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?
"Test Plan" is required.
Sounds like a good practice to me. Worst case scenario, type "I didn't test this patch at all."
".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.
"Lint" and "Unit" are shown as completely different processes. Diffs all over the page clutter the UI. No powerful plain-text Gerrit-like queries. I have to click "Edit Revision" to add reviewers. No -2/-1/+1/+2. WTF?
See "Tokens!" below. :) Discuss: https://phabricator.wikimedia.org/T138
</rant> <yay> Tokens! Comment preview! Can paste raw diffs! </yay> <summary> Some nice features aren't worth a change of workflow. </summary> </review>
On Thu, May 21, 2015 at 4:43 AM, Quim Gil qgil@wikimedia.org wrote:
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.
We also allow for self-merging of documentation fixes that don't change any code (e.g. "Oops, I forgot the RELEASE-NOTES entry!").
And if I manually rebase someone else's patch, or amend it to clean up whitespace, or cherry-pick it to another branch (automatically? manually because of a merge conflict?), would it let me merge or count that as "my own" now?
I've also had opportunity for self-merge when someone else +2s, but Jenkins decides to be stupid and either not run or have a bogus failure that needs another +2 to tell it to try again. For that matter, I've had opportunity for using "Submit and merge" when Jenkins is being broken or excessively slow during a SWAT deployment.
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
Hi!
"Test Plan" is required.
Sounds like a good practice to me. Worst case scenario, type "I didn't test this patch at all."
I think it's not a good idea, as it trains people to do the opposite thing of what it's intended to train people for. I.e. "mandatory, but put 'whatever' there" IMHO is worse than non-mandatory but supported by common consensus. If we're setting up a new system, we shouldn't set it up so we constantly work around it.
Neither should you, that is the point of code review. Then again, if there
In theory, this is true. In practice, there are numerous occasions where people have to self-+2 - typoes, forgotten files, CI glitches, rebases, etc. Well, ok, "have to" is a strong word here - all of it can be worked around by dragging in somebody and asking them "please +2 this" - but again, that would be working against the setup and also training people that the system sucks and they have to work around it to be effective.
On Thu, May 21, 2015 at 7:05 PM, Stas Malyshev smalyshev@wikimedia.org wrote:
Neither should you, that is the point of code review. Then again, if there
In theory, this is true. In practice, there are numerous occasions where people have to self-+2 - typoes, forgotten files, CI glitches, rebases, etc. Well, ok, "have to" is a strong word here - all of it can be worked around by dragging in somebody and asking them "please +2 this" - but again, that would be working against the setup and also training people that the system sucks and they have to work around it to be effective.
For better or worse, self+2 is not uncommon in Ops workflow today in repos like ops/puppet. We really do try to get other-reviews when we're unsure about things (or they're complicated, or involve new design work, etc...) but there are also many common cases where simple things need doing and nobody can be bothered interrupting themselves and someone else to sync up on review. It's a question of trusting +2-ers with sound judgement: they should know when self+2 is a reasonable option and when it isn't (but good luck documenting every possible rationale for it). Usually the induced level of shame for a self+2 that breaks things is sufficient feedback on judgement.
On Thu, May 21, 2015 at 12:05 PM, Stas Malyshev smalyshev@wikimedia.org wrote:
"Test Plan" is required.
Sounds like a good practice to me. Worst case scenario, type "I didn't
test
this patch at all."
I think it's not a good idea, as it trains people to do the opposite thing of what it's intended to train people for. I.e. "mandatory, but put 'whatever' there" IMHO is worse than non-mandatory but supported by common consensus. If we're setting up a new system, we shouldn't set it up so we constantly work around it.
I don't agree, or I don't understand, "Test Plan:" is fantastic and I've wanted it in commit messages for years. Thinking about how to test is essential to writing good code, and a reminder to express your thought process is invaluable, whether it's "qunit tests now pass", or "it should no longer break when you visit the special page", or "obvious fix". Expecting reviewers to be Nostradamus and infer what the engineer might have thought about testing, or whether the engineer tested at all, is dumb. If someone writes "Test Plan: whatever", they'll get -1s until they respect reviewers enough to write "Test Plan: trivial untested fix in an area that lacks tests".
In the last week my instance crashed on two checkins that were +2'd but never run; both useful improvements to MediaWiki-Vagrant roles that looked completely legit. I appreciate people writing needed code and others reviewing it promptly, but typing: "Test Plan: I didn't test this patch at all" might have saved me 20 minutes of wondering where I screwed up, re-provisioning, hunting down log files, etc. (Both are fixed BTW.)
Can/will we add a "Test Plan: " line to the default commit.message https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#idp25045696 just to get developers in the mood and encourage a standard format? Independent of Differential and separately from whether we make it mandatory. It seems developers have to do git config --global commit.template ~/path/to/better/git_commit.message , but we could put the file in a MediaWiki project like core
Regards, -- =S Page WMF Tech writer
Hi!
I don't agree, or I don't understand, "Test Plan:" is fantastic and I've wanted it in commit messages for years. Thinking about how to test is essential to writing good code, and a reminder to express your thought
I agree completely. I am in no way against having "Test plan". What I am against - and in this instance as in others - is replacing person's judgement with a mechanism, and forcing to crate a fake "test plan" where one doesn't make sense or can not be provided.
If someone writes "Test Plan: whatever", they'll get -1s until they respect reviewers enough to write "Test Plan: trivial untested fix in an area that lacks tests".
Sometimes they would. Sometimes it's fixing "teh" in the README, and writing an essay about how this does not need testing is redundant. Everybody understand why it doesn't need testing - so filling in mandatory fields would be time spent not doing something productive but working around the system that was set up wrong. Admittedly, it would be a small time, but this things add up. And, on more principled direction, it just should not happen if we can avoid it. We can solve it very easily - just not make that field mandatory. As you correctly explained, if that fix does need the test, it'll be -1-ed until it has the tests, and this system seems to be working so far. I think placing trust in sound judgement of the developers is the best.
In the last week my instance crashed on two checkins that were +2'd but never run; both useful improvements to MediaWiki-Vagrant roles that looked completely legit. I appreciate people writing needed code and others
That will always happen. There's no way to prevent it completely. Any setup that promises this will never happen again is just a delusion - every software has bugs, and sooner or later you'll encounter one of them. That could also happen with "Test plan" - nobody guaranteed it wouldn't say "everything fine", and being +2ed, and still break down.
reviewing it promptly, but typing: "Test Plan: I didn't test this patch at
That I have no objections to.
For Parsoid, we have comprehensive tests. When you fix a bug, you are expected to add a test showing the bug, and verifying that you've fixed it. I don't think it would help us at all to have to write "Test Plan: run the test suite." for every commit.
There are a few issues which are not easily testable with our test infrastructure (which is motivation for us to improve it). Writing a "Test Plan" for those would be unobjectionable, and perhaps helpful. But they are the exception, not the rule.
Which is all to say that mandatory fields are annoying, and we should have the ability on a project-by-project basis to enable/disable these things. --scott
Il 21/05/2015 10:43, Quim Gil ha scritto:
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, Ricordisamoaricordisamoa@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?
It's not hard at all. It just seems hackish and half-baked.
"Test Plan" is required.
Sounds like a good practice to me. Worst case scenario, type "I didn't test this patch at all."
I believe encouraging and enforcing good practices belongs to people, not computer programs. Moreover, this field could easily scare off new contributors who aren't used to such practices yet.
".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.
Again, I don't see why software should ever intrude on community processes. For example, I've never merged a patch of mine into Pywikibot, but I reserve the right to do so in exceptional cases.
"Lint" and "Unit" are shown as completely different processes. Diffs all over the page clutter the UI. No powerful plain-text Gerrit-like queries. I have to click "Edit Revision" to add reviewers. No -2/-1/+1/+2. WTF?
See "Tokens!" below. :) Discuss:https://phabricator.wikimedia.org/T138
</rant> <yay> Tokens! Comment preview! Can paste raw diffs! </yay> <summary> Some nice features aren't worth a change of workflow. </summary> </review>
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
No -2/-1/+1/+2. WTF?
There is a discussion about that here: -> https://phabricator.wikimedia.org/T138
Personally I'm not sure, if i understand Differential correctly, but it doesn't automatically merge the changes? :o
Best, Florian
-----Original-Nachricht----- Betreff: [Wikitech-l] First impression with Differential Datum: Thu, 21 May 2015 09:01:59 +0200 Von: Ricordisamoa ricordisamoa@openmailbox.org An: Wikimedia developers wikitech-l@lists.wikimedia.org
<review> <rant> Arcanist has to be manually cloned from Git and added to $PATH. Really? "Test Plan" is required. ".arcconfig" should be automatically detected on git clone. I can't review my own revisions. "Lint" and "Unit" are shown as completely different processes. Diffs all over the page clutter the UI. No powerful plain-text Gerrit-like queries. I have to click "Edit Revision" to add reviewers. No -2/-1/+1/+2. WTF? </rant> <yay> Tokens! Comment preview! Can paste raw diffs! </yay> <summary> Some nice features aren't worth a change of workflow. </summary> </review> _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
wikitech-l@lists.wikimedia.org