Hi,
Please don't take the following as a rant, but rather as a kind reminder
for people conducting code reviews for NuttX.
Please, respect the author. You don't need to re-write every line of the
patch in comments just because you feel that the variable name could be
different or something is not defined in alphabetical order. The author
may have some other reason to e.g. order variable definitions in the
source files. NuttX has it's coding standard (and nxstyle) as well as
inviolables which must be obeyed. Anything past to that goes easily too far.
Again, please respect the author. There are often different ways of
achieving the same goal or functionality. And it is fine to challenge
and discuss of the alternatives. But if the author explains why he has
ended up with a certain implementation, there is no need to challenge
that endlessly, if it doesn't break things for others.
I am bringing this up, since I have seen many PR's which are important
for me and the organizations which I work for, getting stuck in endless
debate and nitpicking.
Just some time ago, a single ethernet driver, was stuck for a week for
style issues (and the style was complying with NuttX coding standards in
the first place).
As an another example, we would very much like to bring in
CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard
on this for some time, and have it working. Now, even when this work it
is only additions, not breaking anything for FLAT_BUILD users, is stuck
in endless debate where half of the comments show that the reviewer
doesn't know the RISC-V ISA thoroughly, and is referring to how things
are done in ARM world. And the other half is largely useless style
nitpicking.
Again, it is fine to discuss, give comments and suggest alternatives -
just respect and trust the author when he explains why he has taken some
paths in the implementation.
The current ways of inspecting new functionality gets on my nerves, as
at the same time we are getting build script changes and "code
harmonizing" changes with fast pace into mainline, which completely
break things for my own platforms. That is, large changes which for
example touch every board and are not tested on those. I am myself
spending at least a day every week digging through these "what is broken
this time".
Finally, I truly respect and appreciate that people put their time into
reviewing all the stuff! So please, please don't take the above as an
"attack" against anyone. I just want to remind everyone about respect
for the authors.
Thanks,
Jukka
- NuttX github code review practices Jukka Laitinen
-
