Another helpful thing might be to distribute a git commit hook that checks for style violations, that way devs get the error when they try to commit their code locally instead of after a round trip through code review.
On Thu, Jul 05, 2012 at 10:54:07AM +0100, Bert Pareyn wrote:
> Some comments in line. Overall +1
> - Bert
> On 4 Jul 2012, at 18:10, Nicolaas Matthijs wrote:
>
> 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.
>
> Agreed.
>
> 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.
>
> There are various (desktop) tools out there that allow for easy formatting
> of all CSS files at once.
>
> 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.
>
> Agreed. It was useful for a while to be so strict as we were still getting
> used to the styling rules we implemented.
> By now however we should feel comfortable with them and implement them as
> we go.
>
> 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
> [1][email protected]
> [2]http://collab.sakaiproject.org/mailman/listinfo/oae-dev
>
> _______________________________________________
> oae-dev mailing list
> [3][email protected]
> http://collab.sakaiproject.org/mailman/listinfo/oae-dev
>
> References
>
> Visible links
> 1. mailto:[email protected]
> 2. http://collab.sakaiproject.org/mailman/listinfo/oae-dev
> 3. mailto:[email protected]
> _______________________________________________
> oae-dev mailing list
> [email protected]
> http://collab.sakaiproject.org/mailman/listinfo/oae-dev
--
D. Stuart Freeman
Georgia Institute of Technology
signature.asc
Description: Digital signature
_______________________________________________ oae-dev mailing list [email protected] http://collab.sakaiproject.org/mailman/listinfo/oae-dev
