Hi Bert,
Thanks for raising this. I have personally also felt that the code
review regime has been problematic as of late, especially around
styling, spacing and quotes issues. Most pull requests are re-opened
for this reason, even though they might be fine otherwise. Because of
the distributed nature of the team, this often unnecessarily adds days
to the life of a pull request.
First of all, I think we need to do a big pass through the system as
soon as 1.4 is out of the door, to fix all of our spacing, quote and
other "issues". I believe most of this can be scripted.
Other than that, I've been extending the clean javascript test to
search for "){", if(, for(, else{ and double quotes that are outside
of a string and I'm sure we can also check the CSS files for
unexpanded CSS. Once that's merged, we can use those tests to make
sure that there are no styling issues, but we don't let them stop pull
requests as the tests will always call them out. We should also really
automatically run our unit tests every night and perhaps even for each
pull request.
Overall, I think focussing on styling issues is too easy and tempting
to put time into, as proper code review should really be focussing on
catching bugs, APIs not being used (correctly), logical errors, etc.
Hope that helps,
Nicolaas
On 4 Jul 2012, at 10:57, Bert Pareyn wrote:
Hi all,
This mail is mostly targeted to UI Devs but everyone should feel
free to jump in.
Since a few months now the UI Development team has been gradually
implementing code style changes throughout the code base.
The rule of thumb is that you should apply the predefined SDK
styling rules to the code you change, even if you didn't write it
yourself.
As of lately, we've been really strict about this and don't let
anything through because of a double quote or a missing space.
I'd like to propose we stop reopening for styling issues, but
instead comment in the ticket that there were styling issues
remaining and the assignee should pay closer attention to avoiding
those in the future.
The reason for this is that code review as strict as this is
stalling development speed and cutting morale.
Nico has a proposition as well; we change all remaining issues in
one go and create a test that checks for the most common code
styling issues.
We'd preferably do this after 1.4.0 has been released and at a time
where it's most convenient for devs (to avoid loads of merge
conflicts).
I wanted to raise this before the UI Development call tonight, so
everyone can form an opinion and bring it up during that call.
Cheers,
- Bert
_______________________________________________
oae-dev mailing list
[email protected]
http://collab.sakaiproject.org/mailman/listinfo/oae-dev
_______________________________________________
oae-dev mailing list
[email protected]
http://collab.sakaiproject.org/mailman/listinfo/oae-dev
