nuttxpr commented on PR #15594: URL: https://github.com/apache/nuttx/pull/15594#issuecomment-2598416233
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does *not* fully meet the NuttX requirements. While it provides a summary of the change and testing results, it lacks crucial information in the Impact section. Here's a breakdown: **Missing Information:** * **Impact - Is new feature added? Is existing feature changed?**: This is clearly a new feature and should be explicitly stated as "YES - New feature added." * **Impact on user (will user need to adapt to change?)**: While seemingly minor, this *does* impact users. Users now have a new way to define configurations. This needs to be explained. Something like "YES - Users can now define custom configuration options using `-DCONFIG_XXX=value` during the CMake configuration stage." * **Impact on build (will build process change?)**: This *does* impact the build process. "YES - The CMake configuration process now recognizes and handles `-DCONFIG_XXX` options, adding them to the generated `config.h` file." * **Impact on documentation (is update required / provided?)**: Documentation almost certainly needs updating to explain this new feature. If documentation *is* provided in the PR itself, state that. Otherwise, state "YES - Documentation updates are required to explain the usage of `-DCONFIG_XXX` options." **Insufficient Information:** * **Impact (all other sections)**: Simply stating "N/A" is not acceptable. Even if there's no impact, explicitly state "NO - No impact on ...". This shows that the impact was considered. **Recommendations for Improvement:** 1. **Expand the Impact section significantly.** Address all points mentioned above. Be explicit. Don't assume reviewers will understand the implications of the change. 2. **Clarify the benefit of this change.** Why is this new feature useful? What problems does it solve? This should be in the Summary. 3. **Provide more comprehensive testing logs.** The "ci-check" statement is insufficient. Show *relevant* portions of the build logs demonstrating the change works as intended. Include examples with different data types (string, integer, etc.) if applicable. 4. **Consider potential downsides.** Are there any security implications of allowing arbitrary `CONFIG_` defines? Could this lead to naming conflicts? By addressing these points, the PR will be much more likely to be accepted. Clear and thorough documentation of changes is crucial for open-source projects. -- 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
