Hi all,
I took a brief hour and reviewed some PRs in
https://github.com/CPFL/Autoware/pulls from the last couple of months. I've noticed some good things and would like to commend you for:
1. PRs are getting reviewed. Still too little time spent on the reviews but it seems that at least 1 person looks through them.
2. CI is being ran and I have not found an instance where it would be abused.
3. We have 20 functions with tests.
Now the bad:
Someone (e.g. @kfunaoka?) needs to become an ultimate gate keeper. None of the PRs can be merged without his approval. What he should be then strictly looking for:
1. Is code tested? There is still less that 1 percentile (not even a percent) covered with tests:
https://servandogs.gitlab.io/Autoware/.
2. Is code documented? Better also implement a CI check for it. Autoware.Auto already has it.
3. Is PR thoroughly reviewed? E.g. if there is 82 files changed in the PR, there can not be only 25 total comments. Also all discussions on the PR must be answered and resolved before the PR can be merged. Don't just ignore questions.
4. Developers do not know C++. If you look at this question and answer it is clear that we do not have enough expertise to develop a code that is supposed to be running in a car:
https://github.com/CPFL/Autoware/pull/1943#discussion_r252741783.
5. Someone (e.g. @aohsato @kosuke) must be watching over the architecture For instance a PR like this
https://github.com/CPFL/Autoware/pull/1943 where a publisher has been added into many, many nodes can break down the whole system because additional traffic has been added
6. We keep forking code from ROS core and importing into Autoware:
https://github.com/CPFL/Autoware/pull/1791/files. In this case there are parts of rosbag.
7. This PR has introduced 156 files!!! changed in Autoware:
https://github.com/CPFL/Autoware/pull/1950. Could some user possibly with the confidence be updating their develop branch if they saw such a big change?
8. All new features in release 1.9.0
https://github.com/CPFL/Autoware/releases/tag/1.9.0 have no tests and no documentation.
9. Features like this
https://github.com/CPFL/Autoware/pull/1950 should be clearly marked as experimental and go possibly into a separate folder (called e.g. experimental)
10. There are currently 238 opened issues and 68 opened PRs. Someone (e.g. @kfunaoka?) should go through and clean them up a bit?
---
[Visit Topic](
https://discourse.ros.org/t/development-practices-of-autoware-ai/7969/1) or reply to this email to respond.
If you do not want to receive messages from ros-users please use the unsubscribe link below. If you use the one above, you will stop all of ros-users from receiving updates.
______________________________________________________________________________
ros-users mailing list
ros-users@lists.ros.org
http://lists.ros.org/mailman/listinfo/ros-users
Unsubscribe: <
http://lists.ros.org/mailman//options/ros-users>
(text/plain)