nuttxpr commented on PR #2911: URL: https://github.com/apache/nuttx-apps/pull/2911#issuecomment-2557523374
[**\[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 meet the basic NuttX requirements, but is missing some crucial information. Here's a breakdown of what's good and what needs improvement: **Strengths:** * **Clear Summary:** The summary provides a good overview of the change, its purpose, and how it works. The link to the related PR is also helpful. * **Impact Section Started:** Addressing the impact is started, but needs significant expansion. * **Testing Information Started:** Mentioning the target and command used is a good start. **Weaknesses:** * **Incomplete Impact Assessment:** The impact section is far too brief. Every "NO" should be justified. Every "YES" needs a detailed explanation. For example: * **Impact on user:** Even if the user doesn't *need* to adapt, *can* they use this new feature? How? Document it. * **Impact on build:** Does adding this target affect build times? Add dependencies? Require new tools? * **Impact on hardware:** While it might not change hardware *behavior*, does it increase code size? Does it require more RAM during the build process? * **Impact on documentation:** If this is a user-facing feature, it absolutely needs documentation. State where the documentation will be added/updated. * **Security, Compatibility:** Explicitly stating "NO" with a brief justification is better than leaving it blank. E.g., "Impact on security: NO. This change only adds a diagnostic tool and does not affect the security of the system itself." * **Insufficient Testing Information:** "esp32c6-devkitc:nsh" and `make debug_info` is a start, but insufficient. Provide: * **Build Host Details:** OS, CPU architecture, compiler version. * **Example `sysinfo.h` file:** Include a *before* and *after* `sysinfo.h` (or snippets) to demonstrate the tool's output. This proves the tool functions correctly. * **`make debug_info` output:** Show the actual output of the command. Does it print anything to the console? Where is the parsed information displayed? * **"Testing logs before change":** What was the previous behavior? What problem did this PR solve? Show how the lack of this tool made debugging difficult. **In short:** The PR description is a good starting point, but needs significantly more detail in the Impact and Testing sections to be considered complete and ready for review. Providing concrete examples and justifications for each point will greatly increase the likelihood of quick and positive feedback. -- 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
