nuttxpr commented on PR #15304: URL: https://github.com/apache/nuttx/pull/15304#issuecomment-2557512557
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. Yes, this PR appears to mostly meet the NuttX requirements, but some sections need more detail. Here's a breakdown of what's good and what needs improvement: **Good:** * **Clear Summary:** The summary explains the "why," "what," and "how" of the change. The link to the related NuttX Apps PR is also helpful. * **Impact - Common Layer Change:** This identifies the general area affected. * **Testing - Specific Configuration:** Mentioning `esp32c6-devkitc:nsh` and `make debug_info` gives some context. **Needs Improvement:** * **Impact:** The Impact section needs significantly more detail. While "Common layer change" is a start, explicitly answer *all* the questions with "YES" or "NO" and provide descriptions where necessary. Even if the answer is "NO," state it explicitly. For example: * **Is new feature added?** YES (debug_info target) * **Is existing feature changed?** NO * **Impact on user...** YES (Users can now run `make debug_info` to get system information.) * **Impact on build...** YES (New `debug_info` target added to the build system.) * **Impact on hardware...** NO * **Impact on documentation...** YES (Documentation should be updated to explain the new `debug_info` target and its usage.) * **Impact on security...** NO (Assuming the sysinfo.h file doesn't contain sensitive data unintentionally.) * **Impact on compatibility...** NO (Assuming no breaking changes.) * **Testing:** The Testing section is the weakest. "Testing logs before change" and "Testing logs after change" are required. Show the actual output of `make debug_info` before the change (which would likely be an error or nothing) and after the change (showing the parsed `sysinfo.h` information). This demonstrates that the new functionality works as expected. Also include details about your build host environment. **Example of Improved Testing Section:** ``` I confirm that changes are verified on local setup and works as intended: * Build Host(s): macOS Ventura 13.5, Intel Core i7, GCC 12.2 * Target(s): esp32c6-devkitc:nsh (simulated) Testing logs before change: ``` make debug_info make: *** No rule to make target 'debug_info'. Stop. ``` Testing logs after change: ``` make debug_info Parsing sysinfo.h... System Information: NuttX Version: v11.0.0 Build Date: 2024-07-24 10:00:00 ARCH: sim BOARD: esp32c6-devkitc CONFIG: nsh ... other sysinfo data ... ``` By providing this level of detail, reviewers can more easily understand the changes and verify their correctness, leading to a smoother review process and a higher quality contribution. -- 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
