nuttxpr commented on PR #16184: URL: https://github.com/apache/nuttx/pull/16184#issuecomment-2796256556
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements *in form*, but it's missing crucial details. While it follows the template structure, it lacks the necessary *content* to be considered complete. Here's a breakdown of what's missing: * **Summary:** While it states *what* was changed, it doesn't explain *why*. What problem did this solve? Why was the old method insufficient? How does the macro approach improve things (e.g., simpler configuration, less code duplication, etc.)? * **Impact:** Just stating "RELEASE" is insufficient. While it might not impact the user or build process significantly, it *does* impact the hardware (Espressif SoCs) and likely requires documentation updates. The PR needs to explicitly address *all* impact points, even if the answer is "NO" with a brief justification. For example: * Impact on user: NO (This is a behind-the-scenes change to improve configuration) * Impact on build: NO (No changes to the build process itself) * Impact on hardware: YES (Affects SPI flash frequency selection on Espressif SoCs) * Impact on documentation: YES (Documentation needs to be updated to reflect the new configuration method) * Impact on security: NO (No security implications) * Impact on compatibility: Potentially YES (Need to specify if this breaks compatibility with existing configurations and how to migrate). * **Testing:** "CI" isn't enough. While CI is important, local testing verification is also required. The PR needs to specify the *specific* host and target environments used for testing, and provide actual *testing logs* (or a link to them if they are extensive) to demonstrate the change works as intended. Simply saying "CI" doesn't provide evidence of thorough testing. In short, the PR needs to be significantly more detailed to be acceptable. It provides a good starting point but needs to fill in the missing information to demonstrate the change is well-motivated, thoroughly tested, and understood in terms of its impact. -- 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
