I like that idea a lot. Most of the violations can easily be caught by
a straightforward regex.
Nicolaas
On 5 Jul 2012, at 17:32, D. Stuart Freeman wrote:
> 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
_______________________________________________
oae-dev mailing list
[email protected]
http://collab.sakaiproject.org/mailman/listinfo/oae-dev
