[Ns-developers] code reviews: a short guide

[Mathieu Lacage]

On Wed, 2010-03-10 at 12:09 +0100, Tommaso Pecorella wrote:

> Part 1) can be easily solved by a generic statement: "follow the
> friggin' coding style conventions or I'm gonna kick you". There's no
> need to loose time by checking code lines one by one. Moreover we have
> a fantastic perl script to do that. It could miss some small bits but
> in my personal opinion there's no need to loose too much time on it.
> Hence, if a reviewer spots a coding style violation, he/she should not
> even bother pointing out where is the violation. The contributor will
> learn to be more careful next time.
> BTW, about this: we really should pass the whole ns-3 repository
> trough that perl script. Enforcing coding style works if there's no

The reason why we have not done this yet is that we have a number of
people working on parallel branches outside of ns-3-dev: if we ran the
style formatter, these people (which includes myself) would have to go
through a major merging nightmare.

>  violation in the already-existing code. Otherwise it's an empty
> statement. How can we reply to a contributor stating "I'll not follow
> conventions since the code base isn't following them either" ?

[snip]

> The doc could or could not have a predefined format, but what I'd like
> to see in it is:
> 1) Rationale
> - what is the issue this code is trying to solve
> - what are the major design choices (if any)
> 
> 2) Use case
> - what the user is expected to do with the code
> 
> 3) Potential issues
> - what is the plan to expand the code to support more scenarios and
> how that could affect the existing code design

+1
> 
> And that (at the end) leads me to asking Mathieu to add another
> "suggestion" to his guide.
> 
> - never try to address future, unknown or potential user's need. Focus
> on right now. If you don't have a clear use case, then you're either
> doing something wrong or you're using in a bad way your time and the
> reviewer's time. Or both.

+1

Mathieu