On Fri, Feb 13, 2015 at 8:50 AM, Liam Wyatt <liamwyatt@gmail.com> wrote:
On 13 February 2015 at 16:42, Brian Wolff <bawolff@gmail.com> wrote:

One thing i would like to see in future gwt development is bringing it further in line with mediawiki coding conventions. There are several parts that do things differently than the normal mediawiki way, and also some parts that duplicate mediawiki code, but is subtly different (e.g. some of the sanitation code stands out in that regard). Ancedotally i believe this inconsistency with MW core code conventions is significantly reducing the amount of patches that interested unaffiliated developers would otherwise contribute. I also think that more closely following mw core conventions would make it easier for a wmf staff (or volunteer) who isnt previously familar with the code to debug critical bugs in emergency situations (e.g. things like the hhvm job runner bug).


Is it feasible for you to identify the specific parts that you're talking about and add them to either the bug is on phabricator, or simply to email them back to me and Dan? While noting that I'm not qualified enough to know, I would think that "ensure consistency with MW core code conventions" is an applicable thing to be added to very first item in this grant proposal ("Identify major bugs in current system and fix"). While these are not "bugs" per-se, if you can identify a hit-list of the places where the current system diverges from standard practice, that would be very helpful. 

Fixing bugs and paying down technical debt are different thing, and the second typically takes much effort than the first (but also has higher benefits in the long term). In my experience, paying down technical debt has a high chance of not happening unless it is explicitly included in the objectives, with dedicated resources.

My top 2 for areas where GWT could benefit from following standard software engineering best practices (admittedly much of MediaWiki core doesn't do these either, especially the older parts, but IMO they are good goals for new code that's intended to be high-quality):

1. Cleaner information flow, with components only passing as much information to each other as needed, passing it explicitly, and, where a large amount of information is needed, passing it as objects with a well-defined structure. GWT currently does most of its information passing via huge unstructured arrays, which has practically the same effect as using globals: methods pass each other the full application state as an array reference, and any method can modify it in any way, which means that changes in the system can have effects on remote and not obviously connected parts, in ways that are very hard to reason about just by reading the code. Looking at a method signature gives no real information about what data the method will use itself, what data it will forward and what data it can change. This is a major pain point in code review as it means that reviewing the classes whose code has been changed is often not enough to understand the effects of the change.

2. Good unit test coverage. Nothing reduces code reviewer anxiety like a solid test suite that is green for the change. This will also become a lot easier if #1is tackled - it is easy to test methods which only depend on their inputs and do that in an obvious way.

On the subject of code review, i believe a major contributing factor to difficulties last time, was development outside of gerrit, and waiting until the end to do code review. I strongly urge that all development be done on wmf's version control (because people watch what happens on wmf version control, and even if nobody is providing code review, they may still point out things that could be significant problems later) . I know first hand that getting code review, especially for non-wmf projects can be extremely difficult. If at all possible, i strongly suggest code review be done as incremently as possible.


I'll take any suggestions for how point might be integrated into the grant application text, but suffice to say that I agree with you, and the plan is always to try and do things 'by the book' and to 'bring people along for the ride'. I think both we (europeana) and the WMF have lessons-learned from the code-review experience last time. I'll let Dan respond with any technical aspects to this point if he wishes?

I wonder if you can make this part of the grant application as non-monetary support, ie. request the WMF to reserve code review time. The development plan outlined in the grant application comes with a significant code review burden (the usual advice in software development methodology books is that code review time be no less than 50% of coding time) and the WMF is, at this time, doing poorly at providing  timely reviews for non-WMF patches (or, for that matter, WMF patches which need to be reviewed by a different team), with average review time being multiple weeks. If code reviews take more than a few days, developers need to multitask between the task they are working on right now and a bunch of older tasks which need to be updated based on reviews, and efficiency goes down. While I agree with Brian that leaving code review to the end makes the problem worse, it is bad either way, and without explicit commitment from the WMF (or someone else with code review rights) you risk significant delays in the project if reviewers have their own high-priority tasks and can't find the time.

(This assumes that you need external code review. Two Europeana devs working on the project and doing code reviews for each other could be an alternative.)