nuttxpr commented on PR #3009: URL: https://github.com/apache/nuttx-apps/pull/3009#issuecomment-2678193264
[**\[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 likely not in *substance*. While it fills out the required sections, the content is very generic and lacks crucial detail. Here's why it's insufficient and how it can be improved: **Summary Issues:** * **Vague:** "Remove spaces from Kconfig files," "Add TABs," etc., are too broad. *Which* Kconfig files? What specific spaces are being removed (leading, trailing, between keywords)? What is the consistent TAB usage being implemented (e.g., 4 spaces)? Why is "help" being replaced with "---help---"? What constitutes "extra TABs"? * **Missing Rationale:** *Why* are these changes being made? Is this for code style consistency, improved readability, fixing a bug, or some other reason? The "Why" is the most important part of the summary. **Impact Issues:** * **Likely Incorrect:** While these changes might *seem* like they have no impact, changes to Kconfig files *can* affect the build process and user experience (e.g., if configurations become unavailable or descriptions are misleading). A more thorough assessment is needed. For example, changing indentation in Kconfig can sometimes break menu structures. Even whitespace changes can impact patching if other changes are based on line/column numbers. * **Too Concise:** Even if there is genuinely no impact, a brief justification (e.g., "No impact on build as these are purely cosmetic changes within Kconfig files") is preferable to just "NO." **Testing Issues:** * **Insufficient Detail:** "local" is meaningless. Specify the OS, architecture, board, and any relevant configuration details. * **Missing Logs:** The log sections are empty. Provide actual log output demonstrating the change's behavior before and after. Even if the change is purely cosmetic, showing the relevant portions of the Kconfig file before and after would be helpful. **How to Improve:** 1. **Be Specific:** Detail the exact changes made, file by file, if necessary. 2. **Explain the "Why":** Clearly articulate the rationale for the changes. Even seemingly trivial changes need justification. For example, "Improving Kconfig file consistency by removing trailing spaces and enforcing a 4-space tab indentation." 3. **Thoroughly Assess Impact:** Think critically about *all* potential consequences, however minor. If you conclude there is no impact, explain why. 4. **Provide Detailed Testing Information:** List *all* relevant host and target details. Include build logs, configuration output, or any other information relevant to verifying the changes. Show *something* changing in the logs, even if it's just whitespace. By addressing these issues, 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
