[This email applies to all orocos subprojects]
I want to start a discussion about ways of improving the quality of
the "official" [*] orocos source code.
Please provide feedback on the following first draft of suggestions,
which should be regarded as complementary with respect to the
guidelines you can already find at
1/ The first basic idea is that each subproject has
- a single release manager RM (who makes final decisions)
- a limited list of people having commit-access to that subproject
(further referred to as "committers". RM is part of "committers")
2/ The second basic idea is that each "non-trivial" [**] commit should
be preceded by a review process [***].
a) Create a bug/feature report on the orocos-bugzilla
b) Submit a patch (e.g. via svn diff --diff-cmd diff -x "-u -p")
A good patch will be approved based on its
* code quality and readability (e.g. class/variable names, magic
numbers, ...)
* documentation (doxygen, xml, website [+])
* license info
* addition/modification of the necessary (unit) tests.
* subproject specific requirements (e.g. in case of BFL: add a
ChangeLog entry)
c) Mention on which "platforms" (RTOS-es, Matrix Libraries, Linux
Distro's) the patch was tested
d) Wait until (at least 1)(an) other committer reviews and approves
this code, or until xxx days/hours pass by
e) If you are a committer, you (or another committer) can commit
the code (atomic commits). If not, the committer who
approved the code will.
NOTES:
1/ In case of minor modifications ( trivial < minor < major ),
steps a) c) d) might be superfluous and posting the output of "svn
diff" on the ML might be sufficient. Again, use common sense to
determine whether a modification is minor or not.
2/ To my knowledge, this process is very similar to current
practice of the RTT and BFL subprojects.
Together with this process, I suggest to perform a cleanup operation
on the OCL subproject, in casu:
- Move all KULeuven/PMA (and other) specific code to another
repository folder.
Why?
* Typically the above rules are too strict for mere "application"
code
* There is currently so much code in OCL that people "loose the
forest between the trees". E.g. I think the usefulness of
hardware/demotool/ is very limited for the outside world.
- Review and fix the other code/documentation. Some examples I found
during a 2 minute glance at the repository (examples are randomly
chosen)
* The timer component is not mentioned on the OCL overview page
* The signals subdirectory (albeit not compiled) still uses
control_kernel headers
* Lots of files (eg. hardware/ethercat-demo/EthercatIO.hpp) lack
(doxygen) documentation and licenses.
As said, this is a first draft. Feedback/Additions are welcome!
Klaas
[*] Everything in orocos/trunk/bfl-kdl-ocl-rtt (and of course the
relevant tag and branch-directories)
[**] The people allowed on the committers list should have enough
"common sense" to realise whether a patch is trivial or not.
E.g. improving/adding doxygen documentation can be considered
as trivial, adapting 3 lines of example code might also be
trivial, ... Fixing a bug is probably never trivial. In case of
doubt, ask on this ML.
[***] I _do_ know this generates overhead on the short term, but I'm
convinced this will lead to time gains, more people using orocos,
... on the long term
[+] Typically not part of the patch
Improving orocos code quality
On Thursday 03 January 2008 17:20:44 Klaas Gadeyne wrote::
> [This email applies to all orocos subprojects]
>
> I want to start a discussion about ways of improving the quality of
> the "official" [*] orocos source code.
>
> Please provide feedback on the following first draft of suggestions,
> which should be regarded as complementary with respect to the
> guidelines you can already find at
>
> 1/ The first basic idea is that each subproject has
> - a single release manager RM (who makes final decisions)
> - a limited list of people having commit-access to that subproject
> (further referred to as "committers". RM is part of "committers")
Check.
>
> 2/ The second basic idea is that each "non-trivial" [**] commit should
> be preceded by a review process [***].
> a) Create a bug/feature report on the orocos-bugzilla
Check.
> b) Submit a patch (e.g. via svn diff --diff-cmd diff -x "-u -p")
> A good patch will be approved based on its
> * code quality and readability (e.g. class/variable names, magic
> numbers, ...)
> * documentation (doxygen, xml, website [+])
> * license info
> * addition/modification of the necessary (unit) tests.
> * subproject specific requirements (e.g. in case of BFL: add a
> ChangeLog entry)
Attach the patch to an email or to the Bugzilla list ? For future reference,
bugzilla is better, for quick reviews, an email with inlined code is better.
> c) Mention on which "platforms" (RTOS-es, Matrix Libraries, Linux
> Distro's) the patch was tested
If applicable. Providing a unit test seems to be a better guarantee.
> d) Wait until (at least 1)(an) other committer reviews and approves
> this code, or until xxx days/hours pass by
> e) If you are a committer, you (or another committer) can commit
> the code (atomic commits). If not, the committer who
> approved the code will.
Check.
>
> NOTES:
> 1/ In case of minor modifications ( trivial < minor < major ),
> steps a) c) d) might be superfluous and posting the output of "svn
> diff" on the ML might be sufficient. Again, use common sense to
> determine whether a modification is minor or not.
You can copy/past the diff in the comment window when you close the bug.
Unfortunately, diffs don't show nicely on the drupal pages as the
auto-formatting of the wiki requires a starting ' ' (space) for fixed width
content... I could hack the drupal code such that it shows text starting
with '+' and '-' as fixed width text as well. Frequent posters should be
aware of how their post is formatted on orocos.org.
>
> 2/ To my knowledge, this process is very similar to current
> practice of the RTT and BFL subprojects.
>
> Together with this process, I suggest to perform a cleanup operation
> on the OCL subproject, in casu:
> - Move all KULeuven/PMA (and other) specific code to another
> repository folder.
> Why?
> * Typically the above rules are too strict for mere "application"
> code
> * There is currently so much code in OCL that people "loose the
> forest between the trees". E.g. I think the usefulness of
> hardware/demotool/ is very limited for the outside world.
>
> - Review and fix the other code/documentation. Some examples I found
> during a 2 minute glance at the repository (examples are randomly
> chosen)
> * The timer component is not mentioned on the OCL overview page
>
The reason is that it doesn't have a manual yet.
> * The signals subdirectory (albeit not compiled) still uses
> control_kernel headers
That code should be on a branch until ready for merge with trunk/ocl.
> * Lots of files (eg. hardware/ethercat-demo/EthercatIO.hpp) lack
> (doxygen) documentation and licenses.
Same remark as above.
Peter
Improving orocos code quality
On Thu, 2008-01-03 at 17:20 +0100, Klaas Gadeyne wrote:
> [This email applies to all orocos subprojects]
>
> I want to start a discussion about ways of improving the quality of
> the "official" [*] orocos source code.
[...]
> As said, this is a first draft. Feedback/Additions are welcome!
I think that _guidelines_ are great! And I think they are really
important for the code quality, and also to help new developers to make
patches that are easily accepted.
But please, don't "over regulate" a open process, just set guidelines
use you common sense.
François
Improving orocos code quality
[...]
> I want to start a discussion about ways of improving the quality of
> the "official" [*] orocos source code.
Thanks for taking this initiative, as the maintainer of the BFL subproject a
fully support the initiative.
I personally believe that your suggestions will make reviewing and writing
patches easier and more consistent (just going through the proposed check
list).
[Sideremark:] I want to propose to mention indentation as a prerequisite for
code quality and readibility.
Tinne
Improving orocos code quality
On Fri, 4 Jan 2008, Ruben Smits wrote:
[...]
>> 1/ The first basic idea is that each subproject has
>> - a single release manager RM (who makes final decisions)
>> - a limited list of people having commit-access to that subproject
>> (further referred to as "committers". RM is part of "committers")
>
> I think this list will be very short. Maybe not more than two people
> for each subproject (including the RM). So won't the review procedure
> only slow things down?
Yes, on the short term.
And the question should perhaps be: "Slow down _what_?" I think the
main challenges for orocos ATM are not the fact that we are lacking
features.
Code review leeds to better code, and hence I'm convinced this will
_speed_ up things on the long term. IMO the fact that somebody else (even
if it's only one single person, as you state above) actually reviews _all_
your code is already of large benefit (at least, this is my feeling
about the current procedure in BFL)
[...]
>> - Review and fix the other code/documentation. Some examples I found
>> during a 2 minute glance at the repository (examples are randomly
>> chosen)
>> * The timer component is not mentioned on the OCL overview page
>>
>> * The signals subdirectory (albeit not compiled) still uses
>> control_kernel headers
>> * Lots of files (eg. hardware/ethercat-demo/EthercatIO.hpp) lack
>> (doxygen) documentation and licenses.
>
> It maybe took only a minute or two to find these examples but it takes
> a lot more time to review and fix them. I think the problem here is
> that it takes a lot of time, time we do not always have to spare.
Which proves exactly my point above I think. The longer we are
waiting with this, the more painful it will get.
Thanks for your feedback!
Klaas
Improving orocos code quality
On Fri, 4 Jan 2008, Klaas Gadeyne wrote:
[...]
> And the question should perhaps be: "Slow down _what_?" I think the
> main challenges for orocos ATM are not the fact that we are lacking
> features.
_The_ number one challenge is to make it super easy for interested
newcomers to get a flavour of what orocos could offer them. That means that
there should be (working!) BINARIES TO DOWNLOAD that show at least
something...
Herman
Improving orocos code quality
On Friday 04 January 2008 11:01:50 Herman Bruyninckx wrote:
> On Fri, 4 Jan 2008, Klaas Gadeyne wrote:
>
> [...]
>
> > And the question should perhaps be: "Slow down _what_?" I think the
> > main challenges for orocos ATM are not the fact that we are lacking
> > features.
>
> _The_ number one challenge is to make it super easy for interested
> newcomers to get a flavour of what orocos could offer them. That means that
> there should be (working!) BINARIES TO DOWNLOAD that show at least
> something...
echo "deb http://www.fmtc.be/debian etch main" > /etc/sources.list.d/orocos
apt-get update
apt-get install orocos-ocl-gnulinux-bin
deployer-gnulinux
That should do it if you're on i386. Now, I'm sure you're not convinced that
this commandline shows 'at least something'... But I'm already quite happy
with it for starters.
Peter
Improving orocos code quality
On Wed, 9 Jan 2008, Peter Soetens wrote:
> On Friday 04 January 2008 11:01:50 Herman Bruyninckx wrote:
>> On Fri, 4 Jan 2008, Klaas Gadeyne wrote:
>> [...]
>>> And the question should perhaps be: "Slow down _what_?" I think the
>>> main challenges for orocos ATM are not the fact that we are lacking
>>> features.
>>
>> _The_ number one challenge is to make it super easy for interested
>> newcomers to get a flavour of what orocos could offer them. That means that
>> there should be (working!) BINARIES TO DOWNLOAD that show at least
>> something...
>
> echo "deb http://www.fmtc.be/debian etch main" > /etc/sources.list.d/orocos
> apt-get update
> apt-get install orocos-ocl-gnulinux-bin
> deployer-gnulinux
>
> That should do it if you're on i386. Now, I'm sure you're not convinced that
> this commandline shows 'at least something'... But I'm already quite happy
> with it for starters.
It is not, unfortunately!
You have two kinds of interested people:
(i) the advanced ones, that know already that they need orocos;
(ii) the uninformed ones, that want to learn about what orocos has to offer
to them in half an hour.
(i) is a _very small_ group; (ii) are "the masses". (i) is supported by
your suggestion, but (ii) is completely _not served_ at this moment...
Herman