nuttxpr commented on PR #14963: URL: https://github.com/apache/nuttx/pull/14963#issuecomment-2502536929
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the NuttX requirements, although the testing section could be improved. Here's a breakdown: **Strengths:** * **Clear Summary:** The summary explains the problem (increasingly complex macro checks in `up_testset`), the proposed solution (`CONFIG_ARCH_HAVE_CUSTOM_TESTSET`), and the reasoning behind it (avoiding long macros and accommodating non-upstreamed boards). The explanation regarding architecture-level vs. board-level configuration is helpful. * **Impact Description:** The impact section clearly states the change for users (modifying board config) and avoids unnecessary changes to the core code. * **Addresses a Real Problem:** The proposed change tackles maintainability issues arising from conditional compilation. **Weaknesses:** * **Testing is Insufficient:** "has passed ostest" is not enough detail. You need to provide: * Specific build host information (OS, compiler version, etc.) * Specific target information (architecture, board, configuration) * Actual log output before and after the change, demonstrating the difference in behavior or resource usage. Even if the output is identical, showing the logs proves that the tests still pass with the new configuration. **Recommendations to improve the PR:** 1. **Expand the Testing Section:** Provide the specific details mentioned above. Show the commands used to run the tests. If `ostest` is a suite of tests, list the specific tests within that suite that were run. If there's a noticeable improvement (e.g., reduced code size, faster execution), quantify it. 2. **Consider a Before/After Code Example:** Include a small snippet showing how the `Kconfig` would be modified for a board using a custom `up_testset`. This would enhance clarity for reviewers. By addressing these points, you'll make your PR stronger and easier for reviewers to assess. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
