Code Review Office Hours

Hosted by mmodell on May 12 2016, 8:00 PM - 9:00 PM.


What's this all about

This is an initiative of Wikimedia Developer-Advocacy and Release-Engineering-Team intended to promote quality and timely code reviews on Wikimedia projects. See T128371: Set up Code Review office hours for related discussion and the overview on mediawiki.org Code_Review/Office_Hours

How does it work

If you have a patch that needs attention, please subscribe to this event and post a link to your patch in the comments, then show up on freenode IRC in the #wikimedia-codereview channel during the scheduled office hours. Developers with +2 should be on hand to review patches, provide feedback and merge any acceptable/deployable patches.

Nominated Patches

Suggested byTask and/or Description of patchLink to patch

Other Patches "for-review"

Recurring Event

Event Series
This event repeats every week.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Silly phab doesnt show timezones. So umm when is this?

This event is for Thursdays at 20:00 UTC / 1:00 PM PDT. This is just getting started, we should schedule more time slots to do additional rounds of code reviews.

Yep, see V9#116. It would be great, if we find more timeslots, where we found enough people to make a review hour, so that people who for example live in europe, have time to review stuff at the afternoon too ;). Maybe we should use a central place to discuss more timeslots ;).

mmodell updated the event description. (Show Details)

I'd like to nominate https://gerrit.wikimedia.org/r/#/c/87288/ (which needs a rebase, but already before it seems like noone really felt like reviewing it either).

+1 to https://gerrit.wikimedia.org/r/#/c/87288/ - long time impatiently awaited and very wanted feature

cc: @liangent as an author, @matmarex since we've discussed this several times...

T37378: Support multiple collations at the same time

In preparation for this I took a look at the 3392 most recent patches in Gerrit and crunched some numbers. The users with the most open patchsets are (in this order):
@Paladox with 237 open patches.
@Legoktm with 123 open patches.
@awight with 110 open patches.
Gerrit Patch Uploader with 93 open patches (https://gerrit.wikimedia.org/r/#/q/owner:%22Gerrit+Patch+Uploader%22+status:open,n,z)
@jayvdb with 92 open patches.
@Xqt with 87 open patches.
@Tgr with 69 open patches.
@Ricordisamoa with 65 open patches.
@XZise with 59 open patches.
@Cenarium with 56 open patches.
@cscott with 54 open patches.
@Jdforrester-WMF with 49 open patches.
@MarkAHershberger with 38 open patches.
@Florian with 37 open patches.
@daniel Kinzler with 36 open patches.
@Withoutaname with 35 open patches.
@Dereckson with 34 open patches.
@Catrope with 33 open patches.
@Esanders with 33 open patches.
@Jdlrobson with 32 open patches.

I'd encourage all these devs to join the session and help us merge/abandon/improve/find new owners for as much of these patches as possible.

Heh, maybe we should not do it like SWAT: Try to stop at the end of the hour, instead, start, and try to review at least one hour, so that we go on, if we have enough volunteers to review, and submit. :)

Thx @Jdlrobson for pinging those people :)

@Jdlrobson thanks, didn't realise I had that much open patches. Some can be closed I think, not sure which ones though.

@Ricordisamoa with 65 open patches.

Are you asking for one of them to be reviewed? It could be https://gerrit.wikimedia.org/r/168282 (T68974)

@Luke081515: I don't plan on any formal time limit, however, out of respect for reviewers' time, we should not demand too much from them. :)

As homework, @Paladox I'd definitely recommend abandoning any patchsets that don't need reviews! It will help us identify what you do need reviews for :)

@Ricordisamoa bring any patches you want the party - I'd recommend starting with the most trivial changes you have so we can reduce the number of open patchsets and allow us to see the forest for the trees!

I will happily generate these lists every code review session - hopefully it will give us a sense of how we are doing :)

@Jdlrobson ok, ive already started doing that :)

Thanks everyone, I am quite happy with the initiative that's being taken here. I expect we will make some real progress. Maybe even have a bit of fun and learn something along the way.

☮ See you all tomorrow!

Could I add this https://gerrit.wikimedia.org/r/#/c/227238/ to the list please. Its a partial fix for something and not a total fix but fixes using $GLOBALS

I'm nominating https://gerrit.wikimedia.org/r/#/c/233207/ by @zhuyifei1999, which would be changing multiple aspects of Cascading protection with regards to files.

Paladox updated the event description. (Show Details)

I'd also suggest we take a pass through all the patches over 2 years old: https://gerrit.wikimedia.org/r/#/q/age:2year,n,z

Ive updated the patch that I popose for this.

I am removing my patch since it has been merged.

Thanks for leading this @mmodell! The log for the main scheduled part is at P3063 (original log which is still being captured)

I think this was a good time for conversation about the code review dynamic. A couple of hasty observations:

  • There was at least one patch where a complicated dev environment was needed. These patches often languish in limbo, and are obviously difficult to handle in a 60-90 minute session. Something to consider in the future is coming up with flavors of review (e.g. an hour for patches that require oAuth, an hour for patches that require 2 wikis)
  • One of the patches from @MGChecker got reverted immediately (T101309, rMWda3464badaf9 , reversion: rMWabc68e63787e). That can be a really bad experience for people, compounded by the fact it was the very end of MGChecker's day. (MGChecker, thank you for not throwing your keyboard at us!) We're always struggling with managing the balance between accepting changes quickly and accepting changes safely.

It was a great inaugural run at this, though. It was a great opportunity for us to be looking at the same general set of patches that hadn't previously gotten attention, even if we didn't get all of the way to "+2". This seems like a great format to iteratively improve on.

There was at least one patch where a complicated dev environment was needed. These patches often languish in limbo, and are obviously difficult to handle in a 60-90 minute session. Something to consider in the future is coming up with flavors of review (e.g. an hour for patches that require oAuth, an hour for patches that require 2 wikis)

My proposed solution is T135195, in that case I would try to maintain that.

I'd love to get some reviews on my patches to WhoIsWatching

I have two patchsets which basically just need a merge (hope so):

Suggested byTask and/or Description of patchLink to patch
@Kghblnadd license infohttps://gerrit.wikimedia.org/r/#/c/284753/
@Kghblnadd license info and tidy filehttps://gerrit.wikimedia.org/r/#/c/292578/

hi @Kghbln, this is an old occurrence but I added your patches to today's event page: https://phabricator.wikimedia.org/E204

A yeah I now see that I can forget it for another week. Phabricator is an hour off and showed 22 - 23 so this just ended. Never mind. Cheers

@Mholloway Oops, so there is an new event item every week. Does make sense to me too. Thanks for adding my patches.

EddieGP subscribed.

Adding the same tags as in E206 as this is obviously the same here.