search

j3soon / ros2-essentials

A repo containing essential ROS2 Humble features for controlling Autonomous Mobile Robots (AMRs) and robotic arm manipulators.
https://j3soon.github.io/ros2-essentials/
Apache License 2.0
10 stars 4 forks source link

Unify workspace style #50

Closed j3soon closed 3 months ago

j3soon commented 3 months ago

Fixes: #34 Fixes: #35

Feel free to skip the testing scripts while reviewing, thanks! The testing script code is not organized very well...

j3soon commented 3 months ago

Thanks for help reviewing and testing these changes!

Should we add a simple rules list to let users know which configurations are absolutely off-limits for changes? It seems that the test scripts might still undergo changes, so it’s not necessary to add these now.

I don't think it's worth our time to add these rules right now. These test scripts can be seen as helper indicators to double-check if we accidentally did something wrong or broke something for now.

I find the use of PLACEHOLDER quite interesting and useful. For example, currently, we create new workspaces using CLI tools. If I had known about this approach earlier, I might have tried it. Are there any repositories with a similar style that I can refer to? If not, that’s okay!

The use of PLACEHOLDER + difflib is just a casual way I came up with to make these tests easier to write. There might be a more effective way to handle these validations, but I'm not yet familiar with the best practices for this...

Will future PRs always need to pass tests? There might be special cases where we need to ignore tests. Of course, if it’s possible to pass the tests, I will certainly try my best to do so!

No, PRs are not required to pass these tests. On the other hand, the tests do not support every correct modifications yet.

It’s important to emphasize that our main goal is to enable continued rapid development in the long run. This refactor along with the accompanying tests is preferred, because without these changes, the maintenance cost of the codebase is expected to increase linearly with the number of workspaces. The time invested now will likely save us much more maintenance and debugging time/effort in the future. However, if adding a test or restriction addresses an extremely rare scenario and is unlikely to save time in the future, we should reconsider whether it's worth the effort.

Thanks again for the discussions.