nuttxpr commented on PR #15882: URL: https://github.com/apache/nuttx/pull/15882#issuecomment-2672160980
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements but has some gaps in testing and the summary could be improved. **Strengths:** * **Clear Impact Description:** The impact section is well-populated with "NO" answers, which is perfectly fine if that's truly the case. * **Build Output Included:** Providing the build output is helpful for reviewers. **Weaknesses:** * **Insufficient Testing:** While a build test is performed, it's not enough. The PR explicitly states a lack of hardware for proper testing. This needs to be addressed. Options include: * **Finding someone with the hardware:** Ask in the NuttX community if someone can test the change. * **Using an emulator (if possible):** Explore if QEMU or another emulator supports the target architecture and the LAN9250. * **Creating a more comprehensive build test:** Even without the hardware, more extensive build testing can be done (different configurations, etc.). If the change affects logging, consider a build that enables verbose logging and check the output for correct formatting. * **Vague Summary:** "Changed format specifiers from `%08x` to `PRIx32` for better compatibility with long data types" isn't specific enough. *Which* long data types? What architectures were problematic? Why was `%08x` incorrect? The summary needs more detail to quickly convey the change's purpose. For example: "Fixes build warnings on [list of architectures] caused by using `%08x` with `uint32_t`. Replaces `%08x` with `PRIx32` to ensure correct formatting for 32-bit unsigned integers across all architectures." * **Missing Issue Reference:** If this PR addresses a specific issue, a link to that issue should be included. * **Lack of "Before" Logs:** The testing section asks for "Testing logs before change." Even if the change only prevented warnings, the previous build logs (with the warnings) should be included for comparison. **Recommendations:** 1. **Prioritize Testing:** This is the most critical gap. Find a way to test the change on actual hardware or a suitable emulator. If neither is possible, explain why and significantly expand the build tests to cover as many code paths as feasible. 2. **Improve the Summary:** Rewrite the summary to be more specific and informative. 3. **Add Issue Reference (if applicable):** Link to the related issue if one exists. 4. **Include "Before" Logs:** Add the build logs from before the change, even if they only show warnings. By addressing these points, the PR will be much stronger and more likely to be accepted. -- 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: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
