Hi all,
as you probably know, we mark commits related to extensions (and other things) not used by Wikimedia as "deferred" in CodeReview. I myself work mostly on extensions not used on Wikimedia (for example, SocialProfile) and thus most of my commits are marked as deferred by fellow developers. However, I think that this is evil and we shouldn't do this. I want to have my SocialProfile-related commits (as well as other extension commits) reviewed, but the deferred status doesn't mean "review me later", it means "nobody cares about this revision", or at least currently it means that. Nowadays when a revision is marked as deferred, it's highly unlikely that anyone ever bothers reviewing it. Ideally there should be a way to tag commits, such as commits to extensions not used by WMF, as non-mission critical yet in need of review.
The deployment queue is already long enough and people who are reviewing code for Wikimedia deployment are having a hard time catching up; I don't want to make their work any more difficult than it already is, I just want to have my extensions reviewed to make sure that no glaring errors or security vulnerabilities slip in.
Thanks and regards, -- Jack Phoenix MediaWiki developer
On Sun, Nov 14, 2010 at 11:58 AM, Jack Phoenix jack@countervandalism.net wrote:
I want to have my SocialProfile-related commits (as well as other extension commits) reviewed, but the deferred status doesn't mean "review me later", it means "nobody cares about this revision", or at least currently it means that. Nowadays when a revision is marked as deferred, it's highly unlikely that anyone ever bothers reviewing it. Ideally there should be a way to tag commits, such as commits to extensions not used by WMF, as non-mission critical yet in need of review.
Hi Jack,
Yup, I agree this is a problem. We've got a manual way to tag commits, but it seems to me what we really need is a way of automatically tagging commits based on path. That would allow us to automatically tag trunk commits in phase3 and WMF-deployed extensions as "wmf-deployment" or something along those lines. We'd then probably also need to beef up the tag functionality so that it's possible/easier (?) to search for all "new"+"wmf-deployment" items, as well as fix an apparent bug with the "total number of results" when a tag is selected. We could then change our practice so that instead of marking things as deferred, the people who are focused on wmf-deployment reviews could filter based on that tag.
Before finding someone willing to do this or getting too deep into the implementation details, I think it's worth getting the opinion of those doing the most code reviews whether this would be welcome functionality, and whether it would be an acceptable change in workflow once the functionality is in place. Code reviewers?
Rob
Rob Lanphier wrote:
On Sun, Nov 14, 2010 at 11:58 AM, Jack Phoenix jack@countervandalism.net wrote:
I want to have my SocialProfile-related commits (as well as other extension commits) reviewed, but the deferred status doesn't mean "review me later", it means "nobody cares about this revision", or at least currently it means that. Nowadays when a revision is marked as deferred, it's highly unlikely that anyone ever bothers reviewing it. Ideally there should be a way to tag commits, such as commits to extensions not used by WMF, as non-mission critical yet in need of review.
Hi Jack,
Yup, I agree this is a problem. We've got a manual way to tag commits, but it seems to me what we really need is a way of automatically tagging commits based on path. That would allow us to automatically tag trunk commits in phase3 and WMF-deployed extensions as "wmf-deployment" or something along those lines. We'd then probably also need to beef up the tag functionality so that it's possible/easier (?) to search for all "new"+"wmf-deployment" items, as well as fix an apparent bug with the "total number of results" when a tag is selected. We could then change our practice so that instead of marking things as deferred, the people who are focused on wmf-deployment reviews could filter based on that tag.
Before finding someone willing to do this or getting too deep into the implementation details, I think it's worth getting the opinion of those doing the most code reviews whether this would be welcome functionality, and whether it would be an acceptable change in workflow once the functionality is in place. Code reviewers?
Rob
You are changing the tools but not the problem. Jack wants his extension reviewed. Nobody seem to care about it. Naming it 'without wmf-deployment flag' instead of 'marked as deferred' won't help.
Perhaps having a description attached to the paths, and shown when the revision is viewed would be benefitial: "Extension Foo does X, alpha stage, expect lots of changes" (don't care too much to review, _should_ get a full review later), "Extension Bar allows all wikipedias to magically write articles" (must be reviewed), "Extension Baz is intended for Wikinews filling bug 1234" (should be looked at, but it's not too important)
On Sun, Nov 14, 2010 at 7:04 PM, Platonides Platonides@gmail.com wrote:
You are changing the tools but not the problem. Jack wants his extension reviewed. Nobody seem to care about it. Naming it 'without wmf-deployment flag' instead of 'marked as deferred' won't help.
Agreed.
And deferred doesn't mean that nobody cares like people keep saying. It means that the code reviewer in question doesn't care /right now/. This happens for one of the following reasons 1) i18n commits: we almost never review these in full because they're huge and it's a very tested process (exporting and checking in the changes) 2) It's a non-WMF extension or other piece of code (eg: WikiWord) 3) Your own code from quite some time ago, and nobody has bothered reviewing it (the newer 'old' status might be better for this, however)
Deferred isn't a graveyard, anything can be pulled out of it for review. FWIW, I just marked all the SocialProfile deferred commits back to new since he wants it reviewed.
Generally speaking, I review the following (in no particular order, and (b) overlaps with (a) and (c) sometimes) a) Core changes b) Things I am interested in c) WMF extensions I work on, eg: FlaggedRevs & CodeReview
Most extensions don't fall into this category for me, so I don't bother looking at them. I'm perfectly willing to leave SocialProfile as new if someone indeed will take up review of it. My main issue is leaving things "new" forever that nobody actually reviews.
-Chad
On Sun, Nov 14, 2010 at 3:39 PM, Rob Lanphier robla@wikimedia.org wrote:
Before finding someone willing to do this or getting too deep into the implementation details, I think it's worth getting the opinion of those doing the most code reviews whether this would be welcome functionality, and whether it would be an acceptable change in workflow once the functionality is in place. Code reviewers?
Adding new states is trivial since we got rid of the Enum :)
I'm all for making "deferred" mean "defer this til later" and actually adding a "dontcare" status.
-Chad
Hoi, Do consider that we promote the use of the Wikimedia SVN for MediaWiki software. With extensions in SVN it is easier to provide localisation support for any and all extensions. When an installation has the LocalisationUpdate extension running, new relevant localisations will become available.
The idea of a "dontcare" status is demotivating and it is not at all in line with best practice. Thanks, GerardM
On 15 November 2010 15:08, Chad innocentkiller@gmail.com wrote:
On Sun, Nov 14, 2010 at 3:39 PM, Rob Lanphier robla@wikimedia.org wrote:
Before finding someone willing to do this or getting too deep into the implementation details, I think it's worth getting the opinion of those doing the most code reviews whether this would be welcome functionality, and whether it would be an acceptable change in workflow once the functionality is in place. Code reviewers?
Adding new states is trivial since we got rid of the Enum :)
I'm all for making "deferred" mean "defer this til later" and actually adding a "dontcare" status.
-Chad
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Mon, Nov 15, 2010 at 4:08 PM, Gerard Meijssen gerard.meijssen@gmail.com wrote:
The idea of a "dontcare" status is demotivating and it is not at all in line with best practice.
Ideally, we would indeed review every commit to our SVN. Ideally however, we would be able to review commits to core in a reasonable time.
So yes, it is not best practice. However, you can't force people to care about stuff they don't care about. (Unless you pay them)
"Gerard Meijssen" gerard.meijssen@gmail.com wrote in message news:AANLkTimP5=DRht3bjsuZuZOE8gmFzAr3cEVA7qjqFXgQ@mail.gmail.com...
The idea of a "dontcare" status is demotivating and it is not at all in line with best practice.
It may be demotivating, but it is essentially just calling a spade a spade. The difference between "won't be reviewed" and "will be reviewed at the end of the world" is purely semantic. "Deferred" does currently contain a mixture of "do honestly intend to get round to eventually" and "don't honestly intend to get round to"; but separating them only exposes the question of which types of commit fall (or should fall) into each category, it doesn't answer it.
--HM
On 15/11/10 06:58, Jack Phoenix wrote:
The deployment queue is already long enough and people who are reviewing code for Wikimedia deployment are having a hard time catching up; I don't want to make their work any more difficult than it already is, I just want to have my extensions reviewed to make sure that no glaring errors or security vulnerabilities slip in.
I think that if you want a favour, you should politely ask the specific person you want it from, rather than complaining about a general lack of generosity.
-- Tim Starling
On Sun, Nov 14, 2010 at 4:35 PM, Tim Starling tstarling@wikimedia.org wrote:
On 15/11/10 06:58, Jack Phoenix wrote:
The deployment queue is already long enough and people who are reviewing code for Wikimedia deployment are having a hard time catching up; I don't want to make their work any more difficult than it already is, I just want to have my extensions reviewed to make sure that no glaring errors or security vulnerabilities slip in.
I think that if you want a favour, you should politely ask the specific person you want it from, rather than complaining about a general lack of generosity.
I didn't read Jack's comment as a complaint about anyone's generosity (though admittedly, it's a little tough to parse that sentence). I read it as acknowledging the fact(?) that the review queue is too long to get through without marking some extensions/etc as "deferred". After discussing this with Jack on IRC[1], I encouraged him to send this email.
This seems like a real problem. If Jack wants someone to review his code, should he simply move it back from "deferred" to "new"? What criteria are used to mark something as "deferred" in the first place? I'm assuming it's currently standard practice to mark extensions that aren't slated for deployment by WMF as "deferred"; do I have that right?
Rob [1] http://toolserver.org/~mwbot/logs/%23mediawiki/20101112.txt ...starting at "[17:47:55] <ashley> speaking of things in SVN vs. things used by WMF..."
On Mon, Nov 15, 2010 at 4:56 PM, Rob Lanphier robla@wikimedia.org wrote:
...snip... This seems like a real problem. If Jack wants someone to review his code, should he simply move it back from "deferred" to "new"? What criteria are used to mark something as "deferred" in the first place? I'm assuming it's currently standard practice to mark extensions that aren't slated for deployment by WMF as "deferred"; do I have that right?
Rob
See http://www.mediawiki.org/wiki/Code_Review and http://www.mediawiki.org/wiki/Code_review_guide -Peachey
On 14/11/10 20:58, Jack Phoenix wrote: <snip>
The deployment queue is already long enough and people who are reviewing code for Wikimedia deployment are having a hard time catching up; I don't want to make their work any more difficult than it already is, I just want to have my extensions reviewed to make sure that no glaring errors or security vulnerabilities slip in.
Hello Jack,
I always apply a path filter when browsing the code review backlog. It means I only see *new* revisions in the */trunk/phase3* path (or the CodeReview extensions).
Revision for your extensions can have any status (defered, new, reverted, ok, fixme), it is not that I do not care : I will simply not see them in the backlog!
If you care about your extension code quality, I think we should marks its deferred revisions as 'new' and start reviewing it. This way, if we ever want to enable it on WMF servers, we will avoid the "later, it is not reviewed" drama.
cheers,
wikitech-l@lists.wikimedia.org